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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZyTzNHil-55v7D3r@google.com>
Date: Fri, 1 Nov 2024 08:26:44 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Binbin Wu <binbin.wu@...ux.intel.com>
Cc: Kai Huang <kai.huang@...el.com>, 
	"yuan.yao@...ux.intel.com" <yuan.yao@...ux.intel.com>, "kvm@...r.kernel.org" <kvm@...r.kernel.org>, 
	"pbonzini@...hat.com" <pbonzini@...hat.com>, Xiaoyao Li <xiaoyao.li@...el.com>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Rick Edgecombe <rick.p.edgecombe@...el.com>, 
	Isaku Yamahata <isaku.yamahata@...el.com>
Subject: Re: [PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace generically

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