[PATCH v2 1/3] x86/emulate: add support for {, v}movq xmm, xmm/m64

[PATCH v2 1/3] x86/emulate: add support for {, v}movq xmm, xmm/m64

[Mihai Donțu]

Signed-off-by: Mihai Donțu <mdontu@bitdefender.com>
---
Changed since v1:
 * added a test for vmovq
 * made the tests depend on SSE and AVX, respectively
 * added emulator support for vmovq (0xd6 forces the operand size to
   64bit)
---
 tools/tests/x86_emulator/test_x86_emulator.c | 44 ++++++++++++++++++++++++++++
 xen/arch/x86/x86_emulate/x86_emulate.c       |  9 +++---
 2 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/tools/tests/x86_emulator/test_x86_emulator.c b/tools/tests/x86_emulator/test_x86_emulator.c
index c7f572a..8994149 100644
--- a/tools/tests/x86_emulator/test_x86_emulator.c
+++ b/tools/tests/x86_emulator/test_x86_emulator.c
@@ -697,6 +697,50 @@ int main(int argc, char **argv)
     else
         printf("skipped\n");
 
+    printf("%-40s", "Testing movq %%xmm0,32(%%eax)...");
+    if ( stack_exec && cpu_has_sse )
+    {
+        decl_insn(movq_to_mem2);
+
+        asm volatile ( "pcmpgtb %%xmm0, %%xmm0\n"
+                       put_insn(movq_to_mem2, "movq %%xmm0, 32(%%eax)")
+                       :: );
+
+        *((unsigned long *)res + 4) = 0xbdbdbdbdbdbdbdbd;
+        set_insn(movq_to_mem2);
+        regs.eax = (unsigned long)res;
+        rc = x86_emulate(&ctxt, &emulops);
+        if ( rc != X86EMUL_OKAY || !check_eip(movq_to_mem2) )
+            goto fail;
+        if ( *((unsigned long *)res + 4) )
+            goto fail;
+        printf("okay\n");
+    }
+    else
+        printf("skipped\n");
+
+    printf("%-40s", "Testing vmovq %%xmm1,32(%%eax)...");
+    if ( stack_exec && cpu_has_avx )
+    {
+        decl_insn(vmovq_to_mem);
+
+        asm volatile ( "pcmpgtb %%xmm1, %%xmm1\n"
+                       put_insn(vmovq_to_mem, "vmovq %%xmm1, 32(%%eax)")
+                       :: );
+
+        *((unsigned long *)res + 4) = 0xbdbdbdbdbdbdbdbd;
+        set_insn(vmovq_to_mem);
+        regs.eax = (unsigned long)res;
+        rc = x86_emulate(&ctxt, &emulops);
+        if ( rc != X86EMUL_OKAY || !check_eip(vmovq_to_mem) )
+            goto fail;
+        if ( *((unsigned long *)res + 4) )
+            goto fail;
+        printf("okay\n");
+    }
+    else
+        printf("skipped\n");
+
     printf("%-40s", "Testing movdqu %xmm2,(%ecx)...");
     if ( stack_exec && cpu_has_sse2 )
     {
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index fe594ba..0301235 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -245,7 +245,7 @@ static uint8_t twobyte_table[256] = {
     ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
     ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
     /* 0xD0 - 0xDF */
-    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+    0, 0, 0, 0, 0, 0, ImplicitOps|ModRM, 0, 0, 0, 0, 0, 0, 0, 0, 0,
     /* 0xE0 - 0xEF */
     0, 0, 0, 0, 0, 0, 0, ImplicitOps|ModRM, 0, 0, 0, 0, 0, 0, 0, 0,
     /* 0xF0 - 0xFF */
@@ -4412,6 +4412,7 @@ x86_emulate(
     case 0x7f: /* movq mm,mm/m64 */
                /* {,v}movdq{a,u} xmm,xmm/m128 */
                /* vmovdq{a,u} ymm,ymm/m256 */
+    case 0xd6: /* {,v}movq xmm,xmm/m64 */
     {
         uint8_t *buf = get_stub(stub);
         struct fpu_insn_ctxt fic = { .insn_bytes = 5 };
@@ -4429,9 +4430,9 @@ x86_emulate(
             case vex_66:
             case vex_f3:
                 host_and_vcpu_must_have(sse2);
-                buf[0] = 0x66; /* movdqa */
+                buf[0] = 0x66; /* SSE */
                 get_fpu(X86EMUL_FPU_xmm, &fic);
-                ea.bytes = 16;
+                ea.bytes = (b == 0xd6 ? 8 : 16);
                 break;
             case vex_none:
                 if ( b != 0xe7 )
@@ -4451,7 +4452,7 @@ x86_emulate(
                     ((vex.pfx != vex_66) && (vex.pfx != vex_f3)));
             host_and_vcpu_must_have(avx);
             get_fpu(X86EMUL_FPU_ymm, &fic);
-            ea.bytes = 16 << vex.l;
+            ea.bytes = (b == 0xd6 ? 8 : 16 << vex.l);
         }
         if ( ea.type == OP_MEM )
         {
-- 
2.9.2


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 2/3] x86/emulate: add support of emulating SSE2 instruction {, v}movd mm, m32

[Mihai Donțu]

Found that Windows driver was using a SSE2 instruction MOVD.

Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
Signed-off-by: Mihai Donțu <mdontu@bitdefender.com>
---
Picked from the XenServer 7 patch queue, as suggested by Andrew Cooper
---
 xen/arch/x86/x86_emulate/x86_emulate.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index 0301235..2a56a67 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -204,7 +204,7 @@ static uint8_t twobyte_table[256] = {
     /* 0x60 - 0x6F */
     0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ImplicitOps|ModRM,
     /* 0x70 - 0x7F */
-    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ImplicitOps|ModRM,
+    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ImplicitOps|ModRM, ImplicitOps|ModRM,
     /* 0x80 - 0x87 */
     ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
     ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
@@ -4409,6 +4409,10 @@ x86_emulate(
     case 0x6f: /* movq mm/m64,mm */
                /* {,v}movdq{a,u} xmm/m128,xmm */
                /* vmovdq{a,u} ymm/m256,ymm */
+    case 0x7e: /* movd mm,r/m32 */
+               /* movq mm,r/m64 */
+               /* {,v}movd xmm,r/m32 */
+               /* {,v}movq xmm,r/m64 */
     case 0x7f: /* movq mm,mm/m64 */
                /* {,v}movdq{a,u} xmm,xmm/m128 */
                /* vmovdq{a,u} ymm,ymm/m256 */
@@ -4432,7 +4436,17 @@ x86_emulate(
                 host_and_vcpu_must_have(sse2);
                 buf[0] = 0x66; /* SSE */
                 get_fpu(X86EMUL_FPU_xmm, &fic);
-                ea.bytes = (b == 0xd6 ? 8 : 16);
+                switch ( b )
+                {
+                case 0x7e:
+                    ea.bytes = 4;
+                    break;
+                case 0xd6:
+                    ea.bytes = 8;
+                    break;
+                default:
+                    ea.bytes = 16;
+                }
                 break;
             case vex_none:
                 if ( b != 0xe7 )
@@ -4452,7 +4466,17 @@ x86_emulate(
                     ((vex.pfx != vex_66) && (vex.pfx != vex_f3)));
             host_and_vcpu_must_have(avx);
             get_fpu(X86EMUL_FPU_ymm, &fic);
-            ea.bytes = (b == 0xd6 ? 8 : 16 << vex.l);
+            switch ( b )
+            {
+            case 0x7e:
+                ea.bytes = 4;
+                break;
+            case 0xd6:
+                ea.bytes = 8;
+                break;
+            default:
+                ea.bytes = 16 << vex.l;
+            }
         }
         if ( ea.type == OP_MEM )
         {
--
2.9.2


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 3/3] x86/emulate: added tests for {, v}movd mm, m32

[Mihai Donțu]

Signed-off-by: Mihai Donțu <mdontu@bitdefender.com>
---
 tools/tests/x86_emulator/test_x86_emulator.c | 42 ++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/tools/tests/x86_emulator/test_x86_emulator.c b/tools/tests/x86_emulator/test_x86_emulator.c
index 8994149..e2bd7ce 100644
--- a/tools/tests/x86_emulator/test_x86_emulator.c
+++ b/tools/tests/x86_emulator/test_x86_emulator.c
@@ -650,6 +650,48 @@ int main(int argc, char **argv)
 #define check_eip(which) (regs.eip == (unsigned long)instr + \
                                       (unsigned long)which##_len)
 
+    printf("%-40s", "Testing movd %%mm3,32(%%eax)...");
+    if ( stack_exec && cpu_has_mmx )
+    {
+        decl_insn(movd_to_mem32);
+
+        asm volatile ( "pcmpeqb %%mm3, %%mm3\n"
+                       put_insn(movd_to_mem32, "movd %%mm3, 32(%%eax)")
+                       :: );
+
+        *(res + 8) = 0xbdbdbdbd;
+        set_insn(movd_to_mem32);
+        regs.eax = (unsigned long)res;
+        rc = x86_emulate(&ctxt, &emulops);
+        if ( (rc != X86EMUL_OKAY) || *(res + 8) != 0xffffffff ||
+             !check_eip(movd_to_mem32) )
+            goto fail;
+        printf("okay\n");
+    }
+    else
+        printf("skipped\n");
+
+    printf("%-40s", "Testing vmovd %%xmm1,32(%%eax)...");
+    if ( stack_exec && cpu_has_avx )
+    {
+        decl_insn(vmovd_to_mem32);
+
+        asm volatile ( "pcmpeqb %%xmm1, %%xmm1\n"
+                       put_insn(vmovd_to_mem32, "vmovd %%xmm1, 32(%%eax)")
+                       :: );
+
+        *(res + 8) = 0xbdbdbdbd;
+        set_insn(vmovd_to_mem32);
+        regs.eax = (unsigned long)res;
+        rc = x86_emulate(&ctxt, &emulops);
+        if ( (rc != X86EMUL_OKAY) || *(res + 8) != 0xffffffff ||
+             !check_eip(vmovd_to_mem32) )
+            goto fail;
+        printf("okay\n");
+    }
+    else
+        printf("skipped\n");
+
     printf("%-40s", "Testing movq %mm3,(%ecx)...");
     if ( stack_exec && cpu_has_mmx )
     {
-- 
2.9.2


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/3] x86/emulate: add support for {, v}movq xmm, xmm/m64

[Andrew Cooper]

On 18/07/16 15:30, Mihai Donțu wrote:
> Signed-off-by: Mihai Donțu <mdontu@bitdefender.com>
> ---
> Changed since v1:
>  * added a test for vmovq
>  * made the tests depend on SSE and AVX, respectively
>  * added emulator support for vmovq (0xd6 forces the operand size to
>    64bit)
> ---
>  tools/tests/x86_emulator/test_x86_emulator.c | 44 ++++++++++++++++++++++++++++
>  xen/arch/x86/x86_emulate/x86_emulate.c       |  9 +++---
>  2 files changed, 49 insertions(+), 4 deletions(-)
>
> diff --git a/tools/tests/x86_emulator/test_x86_emulator.c b/tools/tests/x86_emulator/test_x86_emulator.c
> index c7f572a..8994149 100644
> --- a/tools/tests/x86_emulator/test_x86_emulator.c
> +++ b/tools/tests/x86_emulator/test_x86_emulator.c
> @@ -697,6 +697,50 @@ int main(int argc, char **argv)
>      else
>          printf("skipped\n");
>  
> +    printf("%-40s", "Testing movq %%xmm0,32(%%eax)...");
> +    if ( stack_exec && cpu_has_sse )
> +    {
> +        decl_insn(movq_to_mem2);
> +
> +        asm volatile ( "pcmpgtb %%xmm0, %%xmm0\n"
> +                       put_insn(movq_to_mem2, "movq %%xmm0, 32(%%eax)")
> +                       :: );
> +
> +        *((unsigned long *)res + 4) = 0xbdbdbdbdbdbdbdbd;
> +        set_insn(movq_to_mem2);
> +        regs.eax = (unsigned long)res;
> +        rc = x86_emulate(&ctxt, &emulops);
> +        if ( rc != X86EMUL_OKAY || !check_eip(movq_to_mem2) )
> +            goto fail;
> +        if ( *((unsigned long *)res + 4) )
> +            goto fail;
> +        printf("okay\n");
> +    }
> +    else
> +        printf("skipped\n");
> +
> +    printf("%-40s", "Testing vmovq %%xmm1,32(%%eax)...");
> +    if ( stack_exec && cpu_has_avx )
> +    {
> +        decl_insn(vmovq_to_mem);
> +
> +        asm volatile ( "pcmpgtb %%xmm1, %%xmm1\n"
> +                       put_insn(vmovq_to_mem, "vmovq %%xmm1, 32(%%eax)")
> +                       :: );
> +
> +        *((unsigned long *)res + 4) = 0xbdbdbdbdbdbdbdbd;
> +        set_insn(vmovq_to_mem);
> +        regs.eax = (unsigned long)res;
> +        rc = x86_emulate(&ctxt, &emulops);
> +        if ( rc != X86EMUL_OKAY || !check_eip(vmovq_to_mem) )
> +            goto fail;
> +        if ( *((unsigned long *)res + 4) )
> +            goto fail;
> +        printf("okay\n");
> +    }
> +    else
> +        printf("skipped\n");
> +
>      printf("%-40s", "Testing movdqu %xmm2,(%ecx)...");
>      if ( stack_exec && cpu_has_sse2 )
>      {
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
> index fe594ba..0301235 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -245,7 +245,7 @@ static uint8_t twobyte_table[256] = {
>      ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
>      ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
>      /* 0xD0 - 0xDF */
> -    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> +    0, 0, 0, 0, 0, 0, ImplicitOps|ModRM, 0, 0, 0, 0, 0, 0, 0, 0, 0,
>      /* 0xE0 - 0xEF */
>      0, 0, 0, 0, 0, 0, 0, ImplicitOps|ModRM, 0, 0, 0, 0, 0, 0, 0, 0,
>      /* 0xF0 - 0xFF */
> @@ -4412,6 +4412,7 @@ x86_emulate(
>      case 0x7f: /* movq mm,mm/m64 */
>                 /* {,v}movdq{a,u} xmm,xmm/m128 */
>                 /* vmovdq{a,u} ymm,ymm/m256 */
> +    case 0xd6: /* {,v}movq xmm,xmm/m64 */
>      {
>          uint8_t *buf = get_stub(stub);
>          struct fpu_insn_ctxt fic = { .insn_bytes = 5 };
> @@ -4429,9 +4430,9 @@ x86_emulate(
>              case vex_66:
>              case vex_f3:
>                  host_and_vcpu_must_have(sse2);
> -                buf[0] = 0x66; /* movdqa */
> +                buf[0] = 0x66; /* SSE */
>                  get_fpu(X86EMUL_FPU_xmm, &fic);
> -                ea.bytes = 16;
> +                ea.bytes = (b == 0xd6 ? 8 : 16);
>                  break;
>              case vex_none:
>                  if ( b != 0xe7 )
> @@ -4451,7 +4452,7 @@ x86_emulate(
>                      ((vex.pfx != vex_66) && (vex.pfx != vex_f3)));
>              host_and_vcpu_must_have(avx);
>              get_fpu(X86EMUL_FPU_ymm, &fic);
> -            ea.bytes = 16 << vex.l;
> +            ea.bytes = (b == 0xd6 ? 8 : 16 << vex.l);

It would be helpful to bracket (16 << vex.l) to make it more obvious as
to what the operator precedent is.

Otherwise,

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

I can fix this up on commit if there are no other issues in the series.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 2/3] x86/emulate: add support of emulating SSE2 instruction {, v}movd mm, m32

[Andrew Cooper]

On 18/07/16 15:30, Mihai Donțu wrote:
> @@ -4409,6 +4409,10 @@ x86_emulate(
>      case 0x6f: /* movq mm/m64,mm */
>                 /* {,v}movdq{a,u} xmm/m128,xmm */
>                 /* vmovdq{a,u} ymm/m256,ymm */
> +    case 0x7e: /* movd mm,r/m32 */
> +               /* movq mm,r/m64 */
> +               /* {,v}movd xmm,r/m32 */
> +               /* {,v}movq xmm,r/m64 */

This exposes a vulnerability where a guest can clobber local state in
x86_emulate, by specifying registers such as %ebx as the destination.

You must either
1) Move this case up above the fail_if(ea.type != OP_MEM); check, or
2) modify the stub logic to convert a GPR destination to a memory
address pointing into _regs.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 3/3] x86/emulate: added tests for {, v}movd mm, m32

[Andrew Cooper]

On 18/07/16 15:30, Mihai Donțu wrote:
> Signed-off-by: Mihai Donțu <mdontu@bitdefender.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 2/3] x86/emulate: add support of emulating SSE2 instruction {, v}movd mm, m32

[Mihai Donțu]

[-- Attachment #1.1: Type: text/plain, Size: 986 bytes --]

On Monday 18 July 2016 15:57:09 Andrew Cooper wrote:
> On 18/07/16 15:30, Mihai Donțu wrote:
> > @@ -4409,6 +4409,10 @@ x86_emulate(
> >      case 0x6f: /* movq mm/m64,mm */
> >                 /* {,v}movdq{a,u} xmm/m128,xmm */
> >                 /* vmovdq{a,u} ymm/m256,ymm */
> > +    case 0x7e: /* movd mm,r/m32 */
> > +               /* movq mm,r/m64 */
> > +               /* {,v}movd xmm,r/m32 */
> > +               /* {,v}movq xmm,r/m64 */  
> 
> This exposes a vulnerability where a guest can clobber local state in
> x86_emulate, by specifying registers such as %ebx as the destination.
> 
> You must either
> 1) Move this case up above the fail_if(ea.type != OP_MEM); check, or
> 2) modify the stub logic to convert a GPR destination to a memory
> address pointing into _regs.

I'm taking a look at (2) as it feels like the best approach. If I'm not
making any good progress in the coming days, I'll fallback to (1).

Thank you,

-- 
Mihai DONȚU

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel