[PATCH 1/3] KVM: x86/xen: Validate port number in SCHEDOP_poll

[PATCH 1/3] KVM: x86/xen: Validate port number in SCHEDOP_poll

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

We shouldn't allow guests to poll on arbitrary port numbers off the end
of the event channel table.

Fixes: 1a65105a5aba ("KVM: x86/xen: handle PV spinlocks slowpath")
[dwmw2: my bug though; the original version did check the validity as a
 side-effect of an idr_find() which I ripped out in refactoring.]
Reported-by: Michal Luczaj <mhal@rbox.co>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Cc: stable@kernel.org
---
 arch/x86/kvm/xen.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index 2dae413bd62a..dc2f304f2e69 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -954,6 +954,14 @@ static int kvm_xen_hypercall_complete_userspace(struct kvm_vcpu *vcpu)
 	return kvm_xen_hypercall_set_result(vcpu, run->xen.u.hcall.result);
 }
 
+static inline int max_evtchn_port(struct kvm *kvm)
+{
+	if (IS_ENABLED(CONFIG_64BIT) && kvm->arch.xen.long_mode)
+		return EVTCHN_2L_NR_CHANNELS;
+	else
+		return COMPAT_EVTCHN_2L_NR_CHANNELS;
+}
+
 static bool wait_pending_event(struct kvm_vcpu *vcpu, int nr_ports,
 			       evtchn_port_t *ports)
 {
@@ -1042,6 +1050,10 @@ static bool kvm_xen_schedop_poll(struct kvm_vcpu *vcpu, bool longmode,
 			*r = -EFAULT;
 			goto out;
 		}
+		if (ports[i] >= max_evtchn_port(vcpu->kvm)) {
+			*r = -EINVAL;
+			goto out;
+		}
 	}
 
 	if (sched_poll.nr_ports == 1)
@@ -1297,14 +1309,6 @@ int kvm_xen_hypercall(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
-static inline int max_evtchn_port(struct kvm *kvm)
-{
-	if (IS_ENABLED(CONFIG_64BIT) && kvm->arch.xen.long_mode)
-		return EVTCHN_2L_NR_CHANNELS;
-	else
-		return COMPAT_EVTCHN_2L_NR_CHANNELS;
-}
-
 static void kvm_xen_check_poller(struct kvm_vcpu *vcpu, int port)
 {
 	int poll_evtchn = vcpu->arch.xen.poll_evtchn;
-- 
2.35.3

[PATCH 2/3] KVM: x86/xen: Only do in-kernel acceleration of hypercalls for guest CPL0

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

There are almost no hypercalls which are valid from CPL > 0, and definitely
none which are handled by the kernel.

Fixes: 2fd6df2f2b47 ("KVM: x86/xen: intercept EVTCHNOP_send from guests")
Reported-by: Michal Luczaj <mhal@rbox.co>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Cc: stable@kernel.org
---
 arch/x86/kvm/xen.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index dc2f304f2e69..f3098c0e386a 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -1227,6 +1227,7 @@ int kvm_xen_hypercall(struct kvm_vcpu *vcpu)
 	bool longmode;
 	u64 input, params[6], r = -ENOSYS;
 	bool handled = false;
+	u8 cpl;
 
 	input = (u64)kvm_register_read(vcpu, VCPU_REGS_RAX);
 
@@ -1254,9 +1255,17 @@ int kvm_xen_hypercall(struct kvm_vcpu *vcpu)
 		params[5] = (u64)kvm_r9_read(vcpu);
 	}
 #endif
+	cpl = static_call(kvm_x86_get_cpl)(vcpu);
 	trace_kvm_xen_hypercall(input, params[0], params[1], params[2],
 				params[3], params[4], params[5]);
 
+	/*
+	 * Only allow hypercall acceleration for CPL0. The rare hypercalls that
+	 * are permitted in guest userspace can be handled by the VMM.
+	 */
+	if (unlikely(cpl > 0))
+		goto handle_in_userspace;
+
 	switch (input) {
 	case __HYPERVISOR_xen_version:
 		if (params[0] == XENVER_version && vcpu->kvm->arch.xen.xen_version) {
@@ -1291,10 +1300,11 @@ int kvm_xen_hypercall(struct kvm_vcpu *vcpu)
 	if (handled)
 		return kvm_xen_hypercall_set_result(vcpu, r);
 
+handle_in_userspace:
 	vcpu->run->exit_reason = KVM_EXIT_XEN;
 	vcpu->run->xen.type = KVM_EXIT_XEN_HCALL;
 	vcpu->run->xen.u.hcall.longmode = longmode;
-	vcpu->run->xen.u.hcall.cpl = static_call(kvm_x86_get_cpl)(vcpu);
+	vcpu->run->xen.u.hcall.cpl = cpl;
 	vcpu->run->xen.u.hcall.input = input;
 	vcpu->run->xen.u.hcall.params[0] = params[0];
 	vcpu->run->xen.u.hcall.params[1] = params[1];
-- 
2.35.3

Re: [PATCH 2/3] KVM: x86/xen: Only do in-kernel acceleration of hypercalls for guest CPL0

[Sean Christopherson]

On Wed, Nov 23, 2022, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> There are almost no hypercalls which are valid from CPL > 0, and definitely
> none which are handled by the kernel.
> 
> Fixes: 2fd6df2f2b47 ("KVM: x86/xen: intercept EVTCHNOP_send from guests")
> Reported-by: Michal Luczaj <mhal@rbox.co>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> Cc: stable@kernel.org
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

[PATCH 3/3] KVM: Update gfn_to_pfn_cache khva when it moves within the same page

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

In the case where a GPC is refreshed to a different location within the
same page, we didn't bother to update it. Mostly we don't need to, but
since the ->khva field also includes the offset within the page, that
does have to be updated.

Fixes: 982ed0de4753 ("KVM: Reinstate gfn_to_pfn_cache with invalidation support")
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Reviewed-by: Paul Durrant <paul@xen.org>
Cc: stable@kernel.org
---
 virt/kvm/pfncache.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c
index bd4a46aee384..5f83321bfd2a 100644
--- a/virt/kvm/pfncache.c
+++ b/virt/kvm/pfncache.c
@@ -297,7 +297,12 @@ int kvm_gfn_to_pfn_cache_refresh(struct kvm *kvm, struct gfn_to_pfn_cache *gpc,
 	if (!gpc->valid || old_uhva != gpc->uhva) {
 		ret = hva_to_pfn_retry(kvm, gpc);
 	} else {
-		/* If the HVA→PFN mapping was already valid, don't unmap it. */
+		/*
+		 * If the HVA→PFN mapping was already valid, don't unmap it.
+		 * But do update gpc->khva because the offset within the page
+		 * may have changed.
+		 */
+		gpc->khva = old_khva + page_offset;
 		old_pfn = KVM_PFN_ERR_FAULT;
 		old_khva = NULL;
 		ret = 0;
-- 
2.35.3

Re: [PATCH 3/3] KVM: Update gfn_to_pfn_cache khva when it moves within the same page

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1897 bytes --]

On Wed, 2022-11-23 at 00:20 +0000, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> In the case where a GPC is refreshed to a different location within the
> same page, we didn't bother to update it. Mostly we don't need to, but
> since the ->khva field also includes the offset within the page, that
> does have to be updated.
> 
> Fixes: 982ed0de4753 ("KVM: Reinstate gfn_to_pfn_cache with invalidation support")

Hm, wait. That commit wasn't actually broken because at that point the
page offset was included in the uhva too, so the uhva *did* change and
we'd (gratuitously) take the slower path through hva_to_pfn_retry()
when the GPA moved within the same page.

So I think this should actually be:

Fixes: 3ba2c95ea180 ("KVM: Do not incorporate page offset into gfn=>pfn cache user address")

Which means it's only relevant back to v6.0 stable, not all the way
back to v5.17.


> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> Reviewed-by: Paul Durrant <paul@xen.org>
> Cc: stable@kernel.org
> 
> ---
>  virt/kvm/pfncache.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c
> index bd4a46aee384..5f83321bfd2a 100644
> --- a/virt/kvm/pfncache.c
> +++ b/virt/kvm/pfncache.c
> @@ -297,7 +297,12 @@ int kvm_gfn_to_pfn_cache_refresh(struct kvm *kvm, struct gfn_to_pfn_cache *gpc,
>  	if (!gpc->valid || old_uhva != gpc->uhva) {
>  		ret = hva_to_pfn_retry(kvm, gpc);
>  	} else {
> -		/* If the HVA→PFN mapping was already valid, don't unmap it. */
> +		/*
> +		 * If the HVA→PFN mapping was already valid, don't unmap it.
> +		 * But do update gpc->khva because the offset within the page
> +		 * may have changed.
> +		 */
> +		gpc->khva = old_khva + page_offset;
>  		old_pfn = KVM_PFN_ERR_FAULT;
>  		old_khva = NULL;
>  		ret = 0;
> 


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH 3/3] KVM: Update gfn_to_pfn_cache khva when it moves within the same page

[Sean Christopherson]

On Wed, Nov 23, 2022, David Woodhouse wrote:
> On Wed, 2022-11-23 at 00:20 +0000, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > In the case where a GPC is refreshed to a different location within the
> > same page, we didn't bother to update it. Mostly we don't need to, but
> > since the ->khva field also includes the offset within the page, that
> > does have to be updated.
> > 
> > Fixes: 982ed0de4753 ("KVM: Reinstate gfn_to_pfn_cache with invalidation support")
> 
> Hm, wait. That commit wasn't actually broken because at that point the
> page offset was included in the uhva too, so the uhva *did* change and
> we'd (gratuitously) take the slower path through hva_to_pfn_retry()
> when the GPA moved within the same page.
> 
> So I think this should actually be:
> 
> Fixes: 3ba2c95ea180 ("KVM: Do not incorporate page offset into gfn=>pfn cache user address")

Ya.

> Which means it's only relevant back to v6.0 stable, not all the way
> back to v5.17.

Probably a moot point in the long run since that commit was tagged for stable@
too, in order to simplify the fixes that followed.

> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> > Reviewed-by: Paul Durrant <paul@xen.org>
> > Cc: stable@kernel.org
> > 
> > ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

> >  virt/kvm/pfncache.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c
> > index bd4a46aee384..5f83321bfd2a 100644
> > --- a/virt/kvm/pfncache.c
> > +++ b/virt/kvm/pfncache.c
> > @@ -297,7 +297,12 @@ int kvm_gfn_to_pfn_cache_refresh(struct kvm *kvm, struct gfn_to_pfn_cache *gpc,
> >  	if (!gpc->valid || old_uhva != gpc->uhva) {
> >  		ret = hva_to_pfn_retry(kvm, gpc);
> >  	} else {
> > -		/* If the HVA→PFN mapping was already valid, don't unmap it. */
> > +		/*
> > +		 * If the HVA→PFN mapping was already valid, don't unmap it.
> > +		 * But do update gpc->khva because the offset within the page
> > +		 * may have changed.
> > +		 */
> > +		gpc->khva = old_khva + page_offset;

If/when we rework the APIs, another possible approach would be to store only the
the page aligned address, e.g. force the user to pass in offset+len by doing
something like:


	r = kvm_gpc_lock(...);
	if (r)
		return r;

	my_struct = kvm_gpc_kmap(..., offset, len);

Re: [PATCH 1/3] KVM: x86/xen: Validate port number in SCHEDOP_poll

[Sean Christopherson]

On Wed, Nov 23, 2022, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> We shouldn't allow guests to poll on arbitrary port numbers off the end
> of the event channel table.
> 
> Fixes: 1a65105a5aba ("KVM: x86/xen: handle PV spinlocks slowpath")
> [dwmw2: my bug though; the original version did check the validity as a
>  side-effect of an idr_find() which I ripped out in refactoring.]
> Reported-by: Michal Luczaj <mhal@rbox.co>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> Cc: stable@kernel.org
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>