[01/10] x86/emul: Correct the decoding of vlddqu
diff mbox

Message ID 1490608598-11197-2-git-send-email-andrew.cooper3@citrix.com
State New, archived
Headers show

Commit Message

Andrew Cooper March 27, 2017, 9:56 a.m. UTC
vlddqu is encoded with 0xf2 which causes it to fall into the Scalar general
case in x86_decode_twobyte().  However, it really does have just two operands,
so must remain TwoOp

AFL discovered that the instruction c5 5b f0 3c e5 95 0a cd 63 was considered
valid despite it being a two operand instruction and VEX.vvvv having the value
11.  The resulting use in a stub yielded #UD.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>

From manually decoding the instruciton, I believe Xen's interpretation of
disp32(none*8) is correct.  binutils 2.25 (Debian Jessie) yields:

   0:       c5 5b f0 3c e5 95 0a        vlddqu 0x63cd0a95(,%riz,8),%xmm15
   7:       cd 63

where it has accounted for disp32 in its decode of instruction, but failed to
properly move its instruction pointer on.

Intel XED OTOH simply gives up with:

  ERROR: GENERAL_ERROR Could not decode at offset: 0x0 PC: 0x0:
  [C55BF03CE5950ACD63000000000000]
---
 xen/arch/x86/x86_emulate/x86_emulate.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

Comments

Jan Beulich March 27, 2017, 11:24 a.m. UTC | #1
>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote:
> @@ -2332,9 +2333,9 @@ x86_decode_twobyte(
>          if ( vex.pfx == vex_f3 ) /* movq xmm/m64,xmm */
>          {
>      case X86EMUL_OPC_VEX_F3(0, 0x7e): /* vmovq xmm/m64,xmm */
> -            state->desc = DstImplicit | SrcMem | Mov;
> +            state->desc = DstImplicit | SrcMem | TwoOp;

Why? This is a move after all.

> @@ -2374,11 +2375,25 @@ x86_decode_twobyte(
>      case X86EMUL_OPC_VEX_66(0, 0xc4): /* vpinsrw */
>          state->desc = DstReg | SrcMem16;
>          break;
> +
> +    case 0xf0:
> +        ctxt->opcode |= MASK_INSR(vex.pfx, X86EMUL_OPC_PFX_MASK);
> +        if ( vex.pfx == vex_f2 ) /* lddqu mem,xmm */
> +        {
> +        /* fall through */
> +    case X86EMUL_OPC_VEX_F2(0, 0xf0): /* vlddqu mem,{x,y}mm */
> +            state->desc = DstImplicit | SrcMem | TwoOp;

I'd prefer it to be Mov here too, as the insn is a move even if its
name doesn't say so.

Jan
Andrew Cooper March 27, 2017, 12:10 p.m. UTC | #2
On 27/03/17 12:24, Jan Beulich wrote:
>>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote:
>> @@ -2332,9 +2333,9 @@ x86_decode_twobyte(
>>          if ( vex.pfx == vex_f3 ) /* movq xmm/m64,xmm */
>>          {
>>      case X86EMUL_OPC_VEX_F3(0, 0x7e): /* vmovq xmm/m64,xmm */
>> -            state->desc = DstImplicit | SrcMem | Mov;
>> +            state->desc = DstImplicit | SrcMem | TwoOp;
> Why? This is a move after all.
>
>> @@ -2374,11 +2375,25 @@ x86_decode_twobyte(
>>      case X86EMUL_OPC_VEX_66(0, 0xc4): /* vpinsrw */
>>          state->desc = DstReg | SrcMem16;
>>          break;
>> +
>> +    case 0xf0:
>> +        ctxt->opcode |= MASK_INSR(vex.pfx, X86EMUL_OPC_PFX_MASK);
>> +        if ( vex.pfx == vex_f2 ) /* lddqu mem,xmm */
>> +        {
>> +        /* fall through */
>> +    case X86EMUL_OPC_VEX_F2(0, 0xf0): /* vlddqu mem,{x,y}mm */
>> +            state->desc = DstImplicit | SrcMem | TwoOp;
> I'd prefer it to be Mov here too, as the insn is a move even if its
> name doesn't say so.

The fact that TwoOp and Mov are the same constant is confusing (and why
I didn't particularly like its introduction in the first place),
especially in this context where TwoOp is the more important semantic
piece of information.

~Andrew
Jan Beulich March 27, 2017, 12:30 p.m. UTC | #3
>>> On 27.03.17 at 14:10, <andrew.cooper3@citrix.com> wrote:
> On 27/03/17 12:24, Jan Beulich wrote:
>>>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote:
>>> @@ -2332,9 +2333,9 @@ x86_decode_twobyte(
>>>          if ( vex.pfx == vex_f3 ) /* movq xmm/m64,xmm */
>>>          {
>>>      case X86EMUL_OPC_VEX_F3(0, 0x7e): /* vmovq xmm/m64,xmm */
>>> -            state->desc = DstImplicit | SrcMem | Mov;
>>> +            state->desc = DstImplicit | SrcMem | TwoOp;
>> Why? This is a move after all.
>>
>>> @@ -2374,11 +2375,25 @@ x86_decode_twobyte(
>>>      case X86EMUL_OPC_VEX_66(0, 0xc4): /* vpinsrw */
>>>          state->desc = DstReg | SrcMem16;
>>>          break;
>>> +
>>> +    case 0xf0:
>>> +        ctxt->opcode |= MASK_INSR(vex.pfx, X86EMUL_OPC_PFX_MASK);
>>> +        if ( vex.pfx == vex_f2 ) /* lddqu mem,xmm */
>>> +        {
>>> +        /* fall through */
>>> +    case X86EMUL_OPC_VEX_F2(0, 0xf0): /* vlddqu mem,{x,y}mm */
>>> +            state->desc = DstImplicit | SrcMem | TwoOp;
>> I'd prefer it to be Mov here too, as the insn is a move even if its
>> name doesn't say so.
> 
> The fact that TwoOp and Mov are the same constant is confusing (and why
> I didn't particularly like its introduction in the first place),
> especially in this context where TwoOp is the more important semantic
> piece of information.

Well, okay, if we consider that there aren't any real RMW SIMD
insns (albeit for now we emulate a few as such), I can buy this
argument, so

Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Patch
diff mbox

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index bb67be6..497cc77 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -2310,7 +2310,8 @@  x86_decode_twobyte(
     case 0x7f:
     case 0xc2 ... 0xc3:
     case 0xc5 ... 0xc6:
-    case 0xd0 ... 0xfe:
+    case 0xd0 ... 0xef:
+    case 0xf1 ... 0xfe:
         ctxt->opcode |= MASK_INSR(vex.pfx, X86EMUL_OPC_PFX_MASK);
         break;
 
@@ -2332,9 +2333,9 @@  x86_decode_twobyte(
         if ( vex.pfx == vex_f3 ) /* movq xmm/m64,xmm */
         {
     case X86EMUL_OPC_VEX_F3(0, 0x7e): /* vmovq xmm/m64,xmm */
-            state->desc = DstImplicit | SrcMem | Mov;
+            state->desc = DstImplicit | SrcMem | TwoOp;
             state->simd_size = simd_other;
-            /* Avoid the state->desc adjustment below. */
+            /* Avoid the state->desc clobbering of TwoOp below. */
             return X86EMUL_OKAY;
         }
         break;
@@ -2374,11 +2375,25 @@  x86_decode_twobyte(
     case X86EMUL_OPC_VEX_66(0, 0xc4): /* vpinsrw */
         state->desc = DstReg | SrcMem16;
         break;
+
+    case 0xf0:
+        ctxt->opcode |= MASK_INSR(vex.pfx, X86EMUL_OPC_PFX_MASK);
+        if ( vex.pfx == vex_f2 ) /* lddqu mem,xmm */
+        {
+        /* fall through */
+    case X86EMUL_OPC_VEX_F2(0, 0xf0): /* vlddqu mem,{x,y}mm */
+            state->desc = DstImplicit | SrcMem | TwoOp;
+            state->simd_size = simd_other;
+            /* Avoid the state->desc clobbering of TwoOp below. */
+            return X86EMUL_OKAY;
+        }
+        break;
     }
 
     /*
      * Scalar forms of most VEX-encoded TwoOp instructions have
-     * three operands.
+     * three operands.  Those which do really have two operands
+     * should have exited earlier.
      */
     if ( state->simd_size && vex.opcx &&
          (vex.pfx & VEX_PREFIX_SCALAR_MASK) )