Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1384743 > unrolled thread

Re: [PATCH] kvm: x86: do not leak guest xcr0 into host interrupt handlers

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] kvm: x86: do not leak guest xcr0 into host interrupt handlers Wanpeng Li <kernellwp@gmail.com> - 2016-04-22 09:40 +0200
    Re: [PATCH] kvm: x86: do not leak guest xcr0 into host interrupt handlers David Matlack <dmatlack@google.com> - 2016-04-22 19:30 +0200
      Re: [PATCH] kvm: x86: do not leak guest xcr0 into host interrupt handlers Wanpeng Li <kernellwp@gmail.com> - 2016-04-24 00:40 +0200

#1384743 — Re: [PATCH] kvm: x86: do not leak guest xcr0 into host interrupt handlers

Hi Paolo and David,
2016-03-31 3:24 GMT+08:00 David Matlack <dmatlack@google.com>:
> 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 <luto@amacapital.net>
> Signed-off-by: David Matlack <dmatlack@google.com>
> ---
>  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

[toc] | [next] | [standalone]

#1385396

On Fri, Apr 22, 2016 at 12:30 AM, Wanpeng Li <kernellwp@gmail.com> wrote:
> Hi Paolo and David,
> 2016-03-31 3:24 GMT+08:00 David Matlack <dmatlack@google.com>:
>>
>> 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.

You are correct, RFBM[i] will be 0 and XSAVE does not save state
component i in this case. However, XSTATE_BV[i] is left untouched by
XSAVE (left as 1). On XRSTOR, the CPU checks if XSTATE_BV[i] == 1 &&
xcr0[i] == 0, and if so delivers a #GP.

If you are wondering how XSTATE_BV[i] could be 1 in the first place, I
suspect it is left over from a previous XSAVE (which sets XSTATE_BV[i]
to the value in XINUSE[i]).

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

[toc] | [prev] | [next] | [standalone]

#1385734

2016-04-23 1:21 GMT+08:00 David Matlack <dmatlack@google.com>:
> On Fri, Apr 22, 2016 at 12:30 AM, Wanpeng Li <kernellwp@gmail.com> wrote:
>> Hi Paolo and David,
>> 2016-03-31 3:24 GMT+08:00 David Matlack <dmatlack@google.com>:
>>>
>>> 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.
>
> You are correct, RFBM[i] will be 0 and XSAVE does not save state
> component i in this case. However, XSTATE_BV[i] is left untouched by
> XSAVE (left as 1). On XRSTOR, the CPU checks if XSTATE_BV[i] == 1 &&
> xcr0[i] == 0, and if so delivers a #GP.

However, SDM also mentioned that "If RFBM[i] = 0, XRSTOR does not
update state component i." So we #GP on a don't need restore bit i if
XSTATE_BV[I] == 1 && xcr0[0] ==0. That's where I miss I think, thanks
for your explanation.

Regard,
Wanpeng Li

>
> If you are wondering how XSTATE_BV[i] could be 1 in the first place, I
> suspect it is left over from a previous XSAVE (which sets XSTATE_BV[i]
> to the value in XINUSE[i]).

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web