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

From: Sean Christopherson
Date: Fri Nov 01 2024 - 11:27:03 EST


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).