Re: [PATCH 2/2] KVM: fix missing "illegal instruction"-trap in protected modes

[Marcelo Tosatti <mtosatti@redhat.com>]

On Tue, Jan 10, 2012 at 03:26:49PM +0100, Stephan Bärwolf wrote:
> >From 2168285ffb30716f30e129c3ce98ce42d19c4d4e Mon Sep 17 00:00:00 2001
> From: Stephan Baerwolf <stephan.baerwolf@tu-ilmenau.de>
> Date: Sun, 8 Jan 2012 02:03:47 +0000
> Subject: [PATCH 2/2] KVM: fix missing "illegal instruction"-trap in
> protected modes
> 
> On hosts without this patch, 32bit guests will crash
> (and 64bit guests may behave in a wrong way) for
> example by simply executing following nasm-demo-application:
> 
>     [bits 32]
>     global _start
>     SECTION .text
>     _start: syscall
> 
> (I tested it with winxp and linux - both always crashed)
> 
>     Disassembly of section .text:
> 
>     00000000 <_start>:
>        0:   0f 05                   syscall
> 
> The reason seems a missing "invalid opcode"-trap (int6) for the
> syscall opcode "0f05", which is not available on Intel CPUs
> within non-longmodes, as also on some AMD CPUs within legacy-mode.
> (depending on CPU vendor, MSR_EFER and cpuid)
> 
> Because previous mentioned OSs may not engage corresponding
> syscall target-registers (STAR, LSTAR, CSTAR), they remain
> NULL and (non trapping) syscalls are leading to multiple
> faults and finally crashs.
> 
> Depending on the architecture (AMD or Intel) pretended by
> guests, various checks according to vendor's documentation
> are implemented to overcome the current issue and behave
> like the CPUs physical counterparts.
> 
> (Therefore using Intel's "Intel 64 and IA-32 Architecture Software
> Developers Manual" http://www.intel.com/content/dam/doc/manual/
> 64-ia-32-architectures-software-developer-manual-325462.pdf
> and AMD's "AMD64 Architecture Programmer's Manual Volume 3:
> General-Purpose and System Instructions"
> http://support.amd.com/us/Processor_TechDocs/APM_V3_24594.pdf )
> 
> Screenshots of an i686 testing VM (CORE i5 host) before
> and after applying this patch are available under:
> 
> http://matrixstorm.com/software/linux/kvm/20111229/before.jpg
> http://matrixstorm.com/software/linux/kvm/20111229/after.jpg
> 
> Signed-off-by: Stephan Baerwolf <stephan.baerwolf@tu-ilmenau.de>
> ---
>  arch/x86/include/asm/kvm_emulate.h |   15 ++++++
>  arch/x86/kvm/emulate.c             |   92
> ++++++++++++++++++++++++++++++++++-
>  2 files changed, 104 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_emulate.h
> b/arch/x86/include/asm/kvm_emulate.h
> index b172bf4..5b68c23 100644
> --- a/arch/x86/include/asm/kvm_emulate.h
> +++ b/arch/x86/include/asm/kvm_emulate.h
> @@ -301,6 +301,21 @@ struct x86_emulate_ctxt {
>  #define X86EMUL_MODE_PROT     (X86EMUL_MODE_PROT16|X86EMUL_MODE_PROT32| \
>                     X86EMUL_MODE_PROT64)
>  
> +/* CPUID vendors */
> +#define X86EMUL_CPUID_VENDOR_AuthenticAMD_ebx 0x68747541
> +#define X86EMUL_CPUID_VENDOR_AuthenticAMD_ecx 0x444d4163
> +#define X86EMUL_CPUID_VENDOR_AuthenticAMD_edx 0x69746e65
> +
> +#define X86EMUL_CPUID_VENDOR_AMDisbetter_ebx 0x69444d41
> +#define X86EMUL_CPUID_VENDOR_AMDisbetter_ecx 0x21726574
> +#define X86EMUL_CPUID_VENDOR_AMDisbetter_edx 0x74656273
> +
> +#define X86EMUL_CPUID_VENDOR_GenuineIntel_ebx 0x756e6547
> +#define X86EMUL_CPUID_VENDOR_GenuineIntel_ecx 0x6c65746e
> +#define X86EMUL_CPUID_VENDOR_GenuineIntel_edx 0x49656e69
> +
> +
> +
>  enum x86_intercept_stage {
>      X86_ICTP_NONE = 0,   /* Allow zero-init to not match anything */
>      X86_ICPT_PRE_EXCEPT,
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index f1e3be1..3357411 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -1877,6 +1877,94 @@ setup_syscalls_segments(struct x86_emulate_ctxt
> *ctxt,
>      ss->p = 1;
>  }
>  
> +static bool em_syscall_isenabled(struct x86_emulate_ctxt *ctxt)
> +{
> +    struct x86_emulate_ops *ops = ctxt->ops;
> +    u64 efer = 0;
> +
> +    /* syscall is not available in real mode                            */
> +    if ((ctxt->mode == X86EMUL_MODE_REAL) ||
> +        (ctxt->mode == X86EMUL_MODE_VM86))
> +        return false;
> +
> +    ops->get_msr(ctxt, MSR_EFER, &efer);
> +    /* check - if guestOS is aware of syscall (0x0f05)                  */
> +    if ((efer & EFER_SCE) == 0) {
> +        return false;
> +    } else {
> +      /* ok, at this point it becomes vendor-specific                   */
> +      /* so first get us an cpuid                                       */
> +      bool vendor;
> +      u32 eax, ebx, ecx, edx;
> +
> +      /* getting the cpu-vendor                                         */
> +      eax = 0x00000000;
> +      ecx = 0x00000000;
> +      if (likely(ops->get_cpuid))
> +          vendor = ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx);
> +      else    vendor = false;
> +
> +      if (likely(vendor)) {
> +
> +        /* AMD AuthenticAMD / AMDisbetter!                              */
> +        if (((ebx==X86EMUL_CPUID_VENDOR_AuthenticAMD_ebx) &&
> +             (ecx==X86EMUL_CPUID_VENDOR_AuthenticAMD_ecx) &&
> +             (edx==X86EMUL_CPUID_VENDOR_AuthenticAMD_edx)) ||
> +            ((ebx==X86EMUL_CPUID_VENDOR_AMDisbetter_ebx) &&
> +             (ecx==X86EMUL_CPUID_VENDOR_AMDisbetter_ecx) &&
> +             (edx==X86EMUL_CPUID_VENDOR_AMDisbetter_edx))) {
> +
> +              /* if cpu is not in longmode...                           */
> +              /* ...check edx bit11 of cpuid 0x80000001                 */
> +              if (ctxt->mode != X86EMUL_MODE_PROT64) {
> +                eax = 0x80000001;
> +                ecx = 0x00000000;
> +                vendor = ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx);
> +                if (likely(vendor)) {
> +                  if (unlikely(((edx >> 11) & 0x1) == 0))
> +                      return false;
> +                } else {
> +                    return false; /* assuming there is no bit 11        */
> +            }
> +              }
> +              goto __em_syscall_enabled_noprotest;
> +        } /* end "AMD" */
> +
> +
> +        /* Intel (GenuineIntel)                                          */
> +        /* remarks: Intel CPUs only support "syscall" in 64bit longmode  */
> +        /*          Also an 64bit guest within a 32bit compat-app running*/
> +        /*          will #UD !!                                          */
> +        /*          While this behaviour can be fixed (by emulating) into*/
> +        /*          an AMD response - CPUs of AMD can't behave like Intel*/
> +        /*          because without an hardware-raised #UD there is no   */
> +        /*          call in em.-mode (see x86_emulate_instruction(...))! */
> +        /* TODO: make AMD-behaviour configurable                         */
> +        if ((ebx==X86EMUL_CPUID_VENDOR_GenuineIntel_ebx) &&
> +            (ecx==X86EMUL_CPUID_VENDOR_GenuineIntel_ecx) &&
> +            (edx==X86EMUL_CPUID_VENDOR_GenuineIntel_edx)) {
> +              if (ctxt->mode != X86EMUL_MODE_PROT64)
> +                  return false;
> +              goto __em_syscall_enabled_noprotest;
> +        } /* end "Intel" */
> +
> +      } /* end vendor is true */
> +
> +    } /* end MSR_EFER check */
> +
> +    /* default:                               */
> +    /* this code wasn't able to process vendor */
> +    /* so apply  Intels stricter rules...      */
> +        pr_err_ratelimited("kvm: %i: unknown vcpu vendor - assuming
> intel\n",
> +                        current->tgid);
> +
> +    if (ctxt->mode == X86EMUL_MODE_PROT64) goto
> __em_syscall_enabled_noprotest;
> +    return false;
> +
> +__em_syscall_enabled_noprotest:
> +    return true;
> +}
> +
>  static int em_syscall(struct x86_emulate_ctxt *ctxt)
>  {
>      struct x86_emulate_ops *ops = ctxt->ops;
> @@ -1885,9 +1973,7 @@ static int em_syscall(struct x86_emulate_ctxt *ctxt)
>      u16 cs_sel, ss_sel;
>      u64 efer = 0;
>  
> -    /* syscall is not available in real mode */
> -    if (ctxt->mode == X86EMUL_MODE_REAL ||
> -        ctxt->mode == X86EMUL_MODE_VM86)
> +    if (!(em_syscall_isenabled(ctxt)))
>          return emulate_ud(ctxt);
>  
>      ops->get_msr(ctxt, MSR_EFER, &efer);

Stephan, 

I think only the 2-line check to inject UD if EFER_SCE is not set is
missing in em_syscall. The guest is responsible for setting a proper
MSR_STAR for EIP only if it enables SCE_EFER.

Can you please test & resend that patch? Thanks.