[PATCH 0/2] KVM: x86: MPX bug fix and cleanup

[PATCH 0/2] KVM: x86: MPX bug fix and cleanup

[Sean Christopherson]

Fix a theoretical MPX+INIT bug found by inspection and do minor cleanup
in the related code.

Sean Christopherson (2):
  KVM: x86: Fix potential put_fpu() w/o load_fpu() on MPX platform
  KVM: x86: Skip zeroing of MPX state on reset event

 arch/x86/kvm/x86.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

-- 
2.24.0

[PATCH 1/2] KVM: x86: Fix potential put_fpu() w/o load_fpu() on MPX platform

[Sean Christopherson]

Unlike most state managed by XSAVE, MPX is initialized to zero on INIT.
Because INITs are usually recognized in the context of a VCPU_RUN call,
kvm_vcpu_reset() puts the guest's FPU so that the FPU state is resident
in memory, zeros the MPX state, and reloads FPU state to hardware.  But,
in the unlikely event that an INIT is recognized during
kvm_arch_vcpu_ioctl_get_mpstate() via kvm_apic_accept_events(),
kvm_vcpu_reset() will call kvm_put_guest_fpu() without a preceding
kvm_load_guest_fpu() and corrupt the guest's FPU state (and possibly
userspace's FPU state as well).

Given that MPX is being removed from the kernel[*], fix the bug with the
simple-but-ugly approach of loading the guest's FPU during
KVM_GET_MP_STATE.

[*] See commit f240652b6032b ("x86/mpx: Remove MPX APIs").

Fixes: f775b13eedee2 ("x86,kvm: move qemu/guest FPU switching out to vcpu_run")
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---

Tagged for stable since failure would trigger a WARN in the FPU subsystem.
I highly doubt anything outside of syzkaller would actually hit this.

 arch/x86/kvm/x86.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 3ed167e039e5..854ae27bb021 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8712,6 +8712,8 @@ int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
 				    struct kvm_mp_state *mp_state)
 {
 	vcpu_load(vcpu);
+	if (kvm_mpx_supported())
+		kvm_load_guest_fpu(vcpu);
 
 	kvm_apic_accept_events(vcpu);
 	if (vcpu->arch.mp_state == KVM_MP_STATE_HALTED &&
@@ -8720,6 +8722,8 @@ int kvm_arch_vcpu_ioctl_get_mpstate(struct kvm_vcpu *vcpu,
 	else
 		mp_state->mp_state = vcpu->arch.mp_state;
 
+	if (kvm_mpx_supported())
+		kvm_put_guest_fpu(vcpu);
 	vcpu_put(vcpu);
 	return 0;
 }
-- 
2.24.0

[PATCH 2/2] KVM: x86: Skip zeroing of MPX state on reset event

[Sean Christopherson]

Don't bother zeroing out MPX state in the guest's FPU on a reset event,
the guest's FPU is always zero allocated and there is no path between
kvm_arch_vcpu_create() and kvm_arch_vcpu_setup() that can lead to guest
MPX state being modified.

No functional change intended.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/x86.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 854ae27bb021..e6f4174f55cd 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9194,15 +9194,14 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 	kvm_async_pf_hash_reset(vcpu);
 	vcpu->arch.apf.halted = false;
 
-	if (kvm_mpx_supported()) {
+	if (kvm_mpx_supported() && init_event) {
 		void *mpx_state_buffer;
 
 		/*
-		 * To avoid have the INIT path from kvm_apic_has_events() that be
-		 * called with loaded FPU and does not let userspace fix the state.
+		 * Temporarily flush the guest's FPU to memory so that zeroing
+		 * out the MPX areas is done using up-to-date state.
 		 */
-		if (init_event)
-			kvm_put_guest_fpu(vcpu);
+		kvm_put_guest_fpu(vcpu);
 		mpx_state_buffer = get_xsave_addr(&vcpu->arch.guest_fpu->state.xsave,
 					XFEATURE_BNDREGS);
 		if (mpx_state_buffer)
@@ -9211,8 +9210,7 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 					XFEATURE_BNDCSR);
 		if (mpx_state_buffer)
 			memset(mpx_state_buffer, 0, sizeof(struct mpx_bndcsr));
-		if (init_event)
-			kvm_load_guest_fpu(vcpu);
+		kvm_load_guest_fpu(vcpu);
 	}
 
 	if (!init_event) {
-- 
2.24.0

Re: [PATCH 2/2] KVM: x86: Skip zeroing of MPX state on reset event

[Paolo Bonzini]

On 09/12/19 21:19, Sean Christopherson wrote:
> Don't bother zeroing out MPX state in the guest's FPU on a reset event,
> the guest's FPU is always zero allocated and there is no path between
> kvm_arch_vcpu_create() and kvm_arch_vcpu_setup() that can lead to guest
> MPX state being modified.
> 
> No functional change intended.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

Makes sense, but it's a bit weird to have INIT reset _less_ state than
RESET...  I've queued patch 1 only for now.

Paolo

> ---
>  arch/x86/kvm/x86.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 854ae27bb021..e6f4174f55cd 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9194,15 +9194,14 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>  	kvm_async_pf_hash_reset(vcpu);
>  	vcpu->arch.apf.halted = false;
>  
> -	if (kvm_mpx_supported()) {
> +	if (kvm_mpx_supported() && init_event) {
>  		void *mpx_state_buffer;
>  
>  		/*
> -		 * To avoid have the INIT path from kvm_apic_has_events() that be
> -		 * called with loaded FPU and does not let userspace fix the state.
> +		 * Temporarily flush the guest's FPU to memory so that zeroing
> +		 * out the MPX areas is done using up-to-date state.
>  		 */
> -		if (init_event)
> -			kvm_put_guest_fpu(vcpu);
> +		kvm_put_guest_fpu(vcpu);
>  		mpx_state_buffer = get_xsave_addr(&vcpu->arch.guest_fpu->state.xsave,
>  					XFEATURE_BNDREGS);
>  		if (mpx_state_buffer)
> @@ -9211,8 +9210,7 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>  					XFEATURE_BNDCSR);
>  		if (mpx_state_buffer)
>  			memset(mpx_state_buffer, 0, sizeof(struct mpx_bndcsr));
> -		if (init_event)
> -			kvm_load_guest_fpu(vcpu);
> +		kvm_load_guest_fpu(vcpu);
>  	}
>  
>  	if (!init_event) {
>