Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753238AbcDVHaO (ORCPT ); Fri, 22 Apr 2016 03:30:14 -0400 Received: from mail-oi0-f66.google.com ([209.85.218.66]:35760 "EHLO mail-oi0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751977AbcDVHaK (ORCPT ); Fri, 22 Apr 2016 03:30:10 -0400 MIME-Version: 1.0 In-Reply-To: <1459365887-146735-1-git-send-email-dmatlack@google.com> References: <1459365887-146735-1-git-send-email-dmatlack@google.com> Date: Fri, 22 Apr 2016 15:30:09 +0800 Message-ID: Subject: Re: [PATCH] kvm: x86: do not leak guest xcr0 into host interrupt handlers From: Wanpeng Li To: David Matlack Cc: kvm , Paolo Bonzini , "linux-kernel@vger.kernel.org" , Andy Lutomirski , stable@vger.kernel.org Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4452 Lines: 125 Hi Paolo and David, 2016-03-31 3:24 GMT+08:00 David Matlack : > An interrupt handler that uses the fpu can kill a KVM VM, if it runs > under the following conditions: > - the guest's xcr0 register is loaded on the cpu > - the guest's fpu context is not loaded > - the host is using eagerfpu > > Note that the guest's xcr0 register and fpu context are not loaded as > part of the atomic world switch into "guest mode". They are loaded by > KVM while the cpu is still in "host mode". > > Usage of the fpu in interrupt context is gated by irq_fpu_usable(). The > interrupt handler will look something like this: > > if (irq_fpu_usable()) { > kernel_fpu_begin(); > > [... code that uses the fpu ...] > > kernel_fpu_end(); > } > > As long as the guest's fpu is not loaded and the host is using eager > fpu, irq_fpu_usable() returns true (interrupted_kernel_fpu_idle() > returns true). The interrupt handler proceeds to use the fpu with > the guest's xcr0 live. > > kernel_fpu_begin() saves the current fpu context. If this uses > XSAVE[OPT], it may leave the xsave area in an undesirable state. > According to the SDM, during XSAVE bit i of XSTATE_BV is not modified > if bit i is 0 in xcr0. So it's possible that XSTATE_BV[i] == 1 and > xcr0[i] == 0 following an XSAVE. How XSAVE save bit i since SDM mentioned that "XSAVE saves state component i if and only if RFBM[i] = 1. "? RFBM[i] will be 0 if XSTATE_BV[i] == 1 && guest xcr0[i] == 0. Regards, Wanpeng Li > > kernel_fpu_end() restores the fpu context. Now if any bit i in > XSTATE_BV == 1 while xcr0[i] == 0, XRSTOR generates a #GP. The > fault is trapped and SIGSEGV is delivered to the current process. > > Only pre-4.2 kernels appear to be vulnerable to this sequence of > events. Commit 653f52c ("kvm,x86: load guest FPU context more eagerly") > from 4.2 forces the guest's fpu to always be loaded on eagerfpu hosts. > > This patch fixes the bug by keeping the host's xcr0 loaded outside > of the interrupts-disabled region where KVM switches into guest mode. > > Cc: stable@vger.kernel.org > Suggested-by: Andy Lutomirski > Signed-off-by: David Matlack > --- > arch/x86/kvm/x86.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index e260ccb..8df1167 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -700,7 +700,6 @@ static int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr) > if ((xcr0 & XFEATURE_MASK_AVX512) != XFEATURE_MASK_AVX512) > return 1; > } > - kvm_put_guest_xcr0(vcpu); > vcpu->arch.xcr0 = xcr0; > > if ((xcr0 ^ old_xcr0) & XFEATURE_MASK_EXTEND) > @@ -6590,8 +6589,6 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > kvm_x86_ops->prepare_guest_switch(vcpu); > if (vcpu->fpu_active) > kvm_load_guest_fpu(vcpu); > - kvm_load_guest_xcr0(vcpu); > - > vcpu->mode = IN_GUEST_MODE; > > srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx); > @@ -6607,6 +6604,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > > local_irq_disable(); > > + kvm_load_guest_xcr0(vcpu); > + > if (vcpu->mode == EXITING_GUEST_MODE || vcpu->requests > || need_resched() || signal_pending(current)) { > vcpu->mode = OUTSIDE_GUEST_MODE; > @@ -6667,6 +6666,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > vcpu->mode = OUTSIDE_GUEST_MODE; > smp_wmb(); > > + kvm_put_guest_xcr0(vcpu); > + > /* Interrupt is enabled by handle_external_intr() */ > kvm_x86_ops->handle_external_intr(vcpu); > > @@ -7314,7 +7315,6 @@ void kvm_load_guest_fpu(struct kvm_vcpu *vcpu) > * and assume host would use all available bits. > * Guest xcr0 would be loaded later. > */ > - kvm_put_guest_xcr0(vcpu); > vcpu->guest_fpu_loaded = 1; > __kernel_fpu_begin(); > __copy_kernel_to_fpregs(&vcpu->arch.guest_fpu.state); > @@ -7323,8 +7323,6 @@ void kvm_load_guest_fpu(struct kvm_vcpu *vcpu) > > void kvm_put_guest_fpu(struct kvm_vcpu *vcpu) > { > - kvm_put_guest_xcr0(vcpu); > - > if (!vcpu->guest_fpu_loaded) { > vcpu->fpu_counter = 0; > return; > -- > 2.8.0.rc3.226.g39d4020 > -- Regards, Wanpeng Li