Re: [PATCH] KVM: nVMX: Fix handling of lmsw instruction

From: Wanpeng Li <kernellwp@gmail.com>
To: "Jan H. Schönherr" <jschoenh@amazon.de>
Cc: "Paolo Bonzini" <pbonzini@redhat.com>,
	"Radim Krčmář" <rkrcmar@redhat.com>,
	"Thomas Gleixner" <tglx@linutronix.de>,
	"Ingo Molnar" <mingo@redhat.com>,
	"H. Peter Anvin" <hpa@zytor.com>,
	"the arch/x86 maintainers" <x86@kernel.org>,
	kvm <kvm@vger.kernel.org>
Subject: Re: [PATCH] KVM: nVMX: Fix handling of lmsw instruction
Date: Mon, 22 May 2017 07:18:20 +0800	[thread overview]
Message-ID: <CANRm+CyZ64a1g_WqTHUnrX_h4eUbPm7aqSWONf90rBGS48Gu3Q@mail.gmail.com> (raw)
In-Reply-To: <1495279376-4340-1-git-send-email-jschoenh@amazon.de>

2017-05-20 19:22 GMT+08:00 Jan H. Schönherr <jschoenh@amazon.de>:
> The decision whether or not to exit from L2 to L1 on an lmsw instruction is
> based on bogus values: instead of using the information encoded within the
> exit qualification, it uses the data also used for the mov-to-cr
> instruction, which boils down to using whatever is in %eax at that point.

Good catch!

>
> Use the correct values instead.
>
> Without this fix, an L1 may not get notified when a 32-bit Linux L2
> switches its secondary CPUs to protected mode; the L1 is only notified on
> the next modification of CR0. This short time window poses a problem, when
> there is some other reason to exit to L1 in between. Then, L2 will be
> resumed in real mode and chaos ensues.
>
> Signed-off-by: Jan H. Schönherr <jschoenh@amazon.de>

Reviewed-by: Wanpeng Li <wanpeng.li@hotmail.com>

> ---
>  arch/x86/kvm/vmx.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index c6f4ad4..116569a 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -7913,11 +7913,13 @@ static bool nested_vmx_exit_handled_cr(struct kvm_vcpu *vcpu,
>  {
>         unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
>         int cr = exit_qualification & 15;
> -       int reg = (exit_qualification >> 8) & 15;
> -       unsigned long val = kvm_register_readl(vcpu, reg);
> +       int reg;
> +       unsigned long val;
>
>         switch ((exit_qualification >> 4) & 3) {
>         case 0: /* mov to cr */
> +               reg = (exit_qualification >> 8) & 15;
> +               val = kvm_register_readl(vcpu, reg);
>                 switch (cr) {
>                 case 0:
>                         if (vmcs12->cr0_guest_host_mask &
> @@ -7972,6 +7974,7 @@ static bool nested_vmx_exit_handled_cr(struct kvm_vcpu *vcpu,
>                  * lmsw can change bits 1..3 of cr0, and only set bit 0 of
>                  * cr0. Other attempted changes are ignored, with no exit.
>                  */
> +               val = (exit_qualification >> LMSW_SOURCE_DATA_SHIFT) & 0x0f;
>                 if (vmcs12->cr0_guest_host_mask & 0xe &
>                     (val ^ vmcs12->cr0_read_shadow))
>                         return true;
> --
> 2.3.1.dirty
>