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

[Sean Christopherson <seanjc@google.com>]

On Fri, Nov 01, 2024, Binbin Wu wrote:
> On 10/31/2024 10:54 PM, Sean Christopherson wrote:
> > 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.
> > 
> > Also completely untested...

...

> >   	case KVM_HC_MAP_GPA_RANGE: {
> >   		u64 gpa = a0, npages = a1, attrs = a2;
> > -		ret = -KVM_ENOSYS;
> > +		*ret = -KVM_ENOSYS;
> >   		if (!user_exit_on_hypercall(vcpu->kvm, KVM_HC_MAP_GPA_RANGE))
> >   			break;
> >   		if (!PAGE_ALIGNED(gpa) || !npages ||
> >   		    gpa_to_gfn(gpa) + npages <= gpa_to_gfn(gpa)) {
> > -			ret = -KVM_EINVAL;
> > +			*ret = -KVM_EINVAL;
> >   			break;
> >   		}
> 
> *ret needs to be set to 0 for this case before returning 0 to caller?

No, because the caller should consume *ret if and only if the function return value
is '1', i.e. iff KVM should resume the guest.  And I think we actually want to
intentionally not touch *ret, because a sufficient smart compiler (or static
analysis tool) should be able to detect that incorrect usage of *ret is consuming
uninitialized data.

> > @@ -10080,13 +10078,13 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
> >   		return 0;
> >   	}
> >   	default:
> > -		ret = -KVM_ENOSYS;
> > +		*ret = -KVM_ENOSYS;
> >   		break;
> >   	}
> >   out:
> >   	++vcpu->stat.hypercalls;
> > -	return ret;
> > +	return 1;
> >   }
> >   EXPORT_SYMBOL_GPL(__kvm_emulate_hypercall);
> > @@ -10094,7 +10092,7 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
> >   {
> >   	unsigned long nr, a0, a1, a2, a3, ret;
> >   	int op_64_bit;
> > -	int cpl;
> > +	int cpl, r;
> >   	if (kvm_xen_hypercall_enabled(vcpu->kvm))
> >   		return kvm_xen_hypercall(vcpu);
> > @@ -10110,10 +10108,9 @@ 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;
> > +	r = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl, &ret);
> > +	if (r <= r)
> A typo here.
> I guess it meant to be "if (r <= ret)" ?

No, "if (r <= 0)", i.e. exit to userspace on 0 or -errno.

> So the combinations will be
> ----------------------------------------------------------------------------
>    |  r  |    ret    | r <= ret |
> ---|-----|-----------|----------|-------------------------------------------
>  1 |  0  |     0     |   true   |  return r, which is 0, exit to userspace
> ---|-----|-----------|----------|-------------------------------------------
>  2 |  1  |     0     |   false  |  set vcpu's RAX and return back to guest
> ---|-----|-----------|----------|-------------------------------------------
>  3 |  1  | -KVM_Exxx |   false  |  set vcpu's RAX and return back to guest
> ---|-----|-----------|----------|-------------------------------------------
>  4 |  1  |  Positive |   true   |  return r, which is 1,
>    |     |     N     |          |  back to guest without setting vcpu's RAX
> ----------------------------------------------------------------------------
> 
> KVM_HC_SEND_IPI, which calls kvm_pv_send_ipi() can hit case 4, which will
> return back to guest without setting RAX. It is different from the current behavior.
> 
> r can be 0 only if there is no other error detected during pre-checks.
> I think it can just check whether r is 0 or not.

Yeah, I just fat fingered the code (and didn't even compile test).