Re: [PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace generically

["Huang, Kai" <kai.huang@intel.com>]

On Thu, 2024-10-31 at 07:54 -0700, Sean Christopherson wrote:
> On Thu, Oct 31, 2024, Kai Huang wrote:
> > > @@ -10111,12 +10111,21 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
> > >  	cpl = kvm_x86_call(get_cpl)(vcpu);
> > >  
> > >  	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
> > > -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
> > > -		/* MAP_GPA tosses the request to the user space. */
> > > +	if (!ret)
> > >  		return 0;
> > >  
> > > -	if (!op_64_bit)
> > > +	/*
> > > +	 * KVM's ABI with the guest is that '0' is success, and any other value
> > > +	 * is an error code.  Internally, '0' == exit to userspace (see above)
> > > +	 * and '1' == success, as KVM's de facto standard return codes are that
> > > +	 * plus -errno == failure.  Explicitly check for '1' as some PV error
> > > +	 * codes are positive values.
> > > +	 */
> > > +	if (ret == 1)
> > > +		ret = 0;
> > > +	else if (!op_64_bit)
> > >  		ret = (u32)ret;
> > > +
> > >  	kvm_rax_write(vcpu, ret);
> > >  
> > >  	return kvm_skip_emulated_instruction(vcpu);
> > > 
> > 
> > It appears below two cases are not covered correctly?
> > 
> > #ifdef CONFIG_X86_64    
> >         case KVM_HC_CLOCK_PAIRING:
> >                 ret = kvm_pv_clock_pairing(vcpu, a0, a1);
> >                 break;
> > #endif
> >         case KVM_HC_SEND_IPI:
> >                 if (!guest_pv_has(vcpu, KVM_FEATURE_PV_SEND_IPI))
> >                         break;
> > 
> >                 ret = kvm_pv_send_ipi(vcpu->kvm, a0, a1, a2, a3, op_64_bit);
> >                 break; 
> > 
> > Looking at the code, seems at least for KVM_HC_CLOCK_PAIRING,
> > kvm_pv_clock_pairing() returns 0 on success, and the upstream behaviour is not
> > routing to userspace but writing 0 to vcpu's RAX and returning to guest.  With
> > the change above, it immediately returns to userspace w/o writing 0 to RAX.
> > 
> > For KVM_HC_SEND_IPI, seems the upstream behaviour is the return value of
> > kvm_pv_send_ipi() is always written to vcpu's RAX reg, and we always just go
> > back to guest.  With your change, the behaviour will be changed to:
> > 
> >   1) when ret == 0, exit to userspace w/o writing  0 to vcpu's RAX,
> >   2) when ret == 1, it is converted to 0 and then written to RAX.
> > 
> > This doesn't look like safe.
> 
> Drat, I managed to space on the cases that didn't explicit set '0'.  Hrm.
> 
> My other idea was have an out-param to separate the return code intended for KVM
> from the return code intended for the guest.  I generally dislike out-params, but
> trying to juggle a return value that multiplexes guest and host values seems like
> an even worse idea.

Yeah this looks fine to me, one comment below ...


[...]

> -	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
> -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
> -		/* MAP_GPA tosses the request to the user space. */
> -		return 0;
> +	r = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl, &ret);
> +	if (r <= r)
> +		return r;

... should be:

	if (r <= 0)
		return r;

?

Another option might be we move "set hypercall return value" code inside
__kvm_emulate_hypercall().  So IIUC the reason to split
__kvm_emulate_hypercall() out is for TDX, and while non-TDX uses RAX to carry
the hypercall return value, TDX uses R10.

We can additionally pass a "kvm_hypercall_set_ret_func" function pointer to
__kvm_emulate_hypercall(), and invoke it inside.  Then we can change
__kvm_emulate_hypercall() to return: 
    < 0 error, 
    ==0 return to userspace, 
    > 0 go back to guest.

I made some attempt below, build test only:

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 6d9f763a7bb9..c48feb62a5d7 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -2179,10 +2179,14 @@ static inline void kvm_clear_apicv_inhibit(struct kvm
*kvm,
        kvm_set_or_clear_apicv_inhibit(kvm, reason, false);
 }
 
-unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
-                                     unsigned long a0, unsigned long a1,
-                                     unsigned long a2, unsigned long a3,
-                                     int op_64_bit, int cpl);
+typedef void (*kvm_hypercall_set_ret_func_t)(struct kvm_vcpu *vcpu,
+               unsigned long val);
+
+int __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
+                           unsigned long a0, unsigned long a1,
+                           unsigned long a2, unsigned long a3,
+                           int op_64_bit, int cpl,
+                           kvm_hypercall_set_ret_func_t set_ret_func);
 int kvm_emulate_hypercall(struct kvm_vcpu *vcpu);
 
 int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 error_code,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 83fe0a78146f..01bdf01cfc79 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9998,10 +9998,11 @@ static int complete_hypercall_exit(struct kvm_vcpu
*vcpu)
        return kvm_skip_emulated_instruction(vcpu);
 }
 
-unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
-                                     unsigned long a0, unsigned long a1,
-                                     unsigned long a2, unsigned long a3,
-                                     int op_64_bit, int cpl)
+int __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
+                           unsigned long a0, unsigned long a1,
+                           unsigned long a2, unsigned long a3,
+                           int op_64_bit, int cpl,
+                           kvm_hypercall_set_ret_func_t set_ret_func)
 {
        unsigned long ret;
 
@@ -10076,6 +10077,7 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu
*vcpu, unsigned long nr,
 
                WARN_ON_ONCE(vcpu->run->hypercall.flags &
KVM_EXIT_HYPERCALL_MBZ);
                vcpu->arch.complete_userspace_io = complete_hypercall_exit;
+               /* MAP_GPA tosses the request to the user space. */
                /* stat is incremented on completion. */
                return 0;
        }
@@ -10085,16 +10087,26 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu
*vcpu, unsigned long nr,
        }
 
 out:
+       if (!op_64_bit)
+               ret = (u32)ret;
+       (*set_ret_func)(vcpu, ret);
+
        ++vcpu->stat.hypercalls;
-       return ret;
+       return 1;
 }
 EXPORT_SYMBOL_GPL(__kvm_emulate_hypercall);
 
+static void kvm_hypercall_set_ret_default(struct kvm_vcpu *vcpu,
+                                         unsigned long val)
+{
+       kvm_rax_write(vcpu, val);
+}
+
 int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
 {
-       unsigned long nr, a0, a1, a2, a3, ret;
+       unsigned long nr, a0, a1, a2, a3;
        int op_64_bit;
-       int cpl;
+       int cpl, ret;
 
        if (kvm_xen_hypercall_enabled(vcpu->kvm))
                return kvm_xen_hypercall(vcpu);
@@ -10110,14 +10122,10 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
        op_64_bit = is_64_bit_hypercall(vcpu);
        cpl = kvm_x86_call(get_cpl)(vcpu);
 
-       ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
-       if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
-               /* MAP_GPA tosses the request to the user space. */
-               return 0;
-
-       if (!op_64_bit)
-               ret = (u32)ret;
-       kvm_rax_write(vcpu, ret);
+       ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl,
+                       kvm_hypercall_set_ret_default);
+       if (ret <= 0)
+               return ret;
 
        return kvm_skip_emulated_instruction(vcpu);
 }