[PATCH] target/i386: Reject VPINSRW with VEX.pp != 0b01

Andrey Polivoda posted 1 patch 1 week ago
target/i386/tcg/decode-new.c.inc | 12 +++++++++++-
target/i386/tcg/decode-new.h     |  1 +
2 files changed, 12 insertions(+), 1 deletion(-)
[PATCH] target/i386: Reject VPINSRW with VEX.pp != 0b01
Posted by Andrey Polivoda 1 week ago
According to both Volume 2B of Intel 64 and IA-32 Architectures Software
Developer's Manual and Volume 4 of AMD64 Architecture Programmer's Manual,
VPINSRW instruction can be encoded with VEX prefix only when VEX.pp == 0b01
(which implies 0x66 prefix).

Currently, QEMU allows decoding VPINSRW with either VEX.pp == 0b00
(which implies that no prefix is used) or VEX.pp == 0b01.

The first case allows the guest code to crash QEMU, since vector_len()
returns 8 in this case, causing the following assertion failure:
../target/i386/tcg/emit.c.inc:3092: void gen_pinsr(DisasContext *, X86DecodedInsn *, MemOp): Assertion `vec_len == 16' failed.

Real hardware, however, simply raises a #UD exception when executing VPINSRW
with VEX.pp == 0b00.

This patch fixes this issue by modifying the decoder table entry for PINSRW
to require the use of 0x66 prefix when VEX prefix is used.

Fixes: aba2b8ecb905 ("target/i386: reimplement 0x0f 0xc2, 0xc4-0xc6, add AVX")
Cc: qemu-stable@nongnu.org
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Andrey Polivoda <apolivodaa433@gmail.com>
---
Note: checkpatch.pl reported a few errors:
1. Decoder table entry line being longer than 80 characters
2. Missing space after colons at the decoder table entry line
These seem to be false positives, since the affected line follows
the style of the decoder table entries.

 target/i386/tcg/decode-new.c.inc | 12 +++++++++++-
 target/i386/tcg/decode-new.h     |  1 +
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/target/i386/tcg/decode-new.c.inc b/target/i386/tcg/decode-new.c.inc
index 3845fab5c8..5b9a4b4fa1 100644
--- a/target/i386/tcg/decode-new.c.inc
+++ b/target/i386/tcg/decode-new.c.inc
@@ -290,6 +290,8 @@
 #define p_66_f3_f2    .valid_prefix = P_66 | P_F3 | P_F2,
 #define p_00_66_f3_f2 .valid_prefix = P_00 | P_66 | P_F3 | P_F2,
 
+#define vex_p_66 .vex_valid_prefix = P_66,
+
 #define UNKNOWN_OPCODE ((X86OpEntry) {})
 
 #define X86_MAX_INSN_LENGTH 15
@@ -1391,7 +1393,7 @@ static const X86OpEntry opcodes_0F[256] = {
     [0xc1] = X86_OP_ENTRY2(XADD,       E,v, G,v,            lock),
     [0xc2] = X86_OP_ENTRY4(VCMP,       V,x, H,x, W,x,       vex2_rep3 p_00_66_f3_f2),
     [0xc3] = X86_OP_ENTRY3(MOV,        EM,y,G,y, None,None, cpuid(SSE2) p_00), /* MOVNTI */
-    [0xc4] = X86_OP_ENTRY4(PINSRW,     V,dq,H,dq,E,w,       vex5 mmx p_00_66),
+    [0xc4] = X86_OP_ENTRY4(PINSRW,     V,dq,H,dq,E,w,       vex5 mmx p_00_66 vex_p_66),
     [0xc5] = X86_OP_ENTRY3(PEXTRW,     G,d, U,dq,I,b,       vex5 mmx p_00_66),
     [0xc6] = X86_OP_ENTRY4(VSHUF,      V,x, H,x, W,x,       vex4 p_00_66),
     [0xc7] = X86_OP_GROUP0(group9),
@@ -2760,6 +2762,9 @@ static bool validate_sse_prefix(DisasContext *s, X86OpEntry *e)
 
         /* fallthrough */
     default:
+        if ((s->prefix & (PREFIX_VEX | PREFIX_EVEX)) && e->vex_valid_prefix) {
+            break;
+        }
         if (!e->valid_prefix) {
             return true;
         }
@@ -2772,6 +2777,11 @@ static bool validate_sse_prefix(DisasContext *s, X86OpEntry *e)
 
     /* Now, either zero or one bit is set in sse_prefixes.  */
     sse_prefixes = s->prefix & (PREFIX_REPZ | PREFIX_REPNZ | PREFIX_DATA);
+
+    if ((s->prefix & (PREFIX_VEX | PREFIX_EVEX)) && e->vex_valid_prefix) {
+        return e->vex_valid_prefix & (1 << sse_prefixes);
+    }
+
     return e->valid_prefix & (1 << sse_prefixes);
 }
 
diff --git a/target/i386/tcg/decode-new.h b/target/i386/tcg/decode-new.h
index 5f76b0c1b7..d021754d9a 100644
--- a/target/i386/tcg/decode-new.h
+++ b/target/i386/tcg/decode-new.h
@@ -325,6 +325,7 @@ struct X86OpEntry {
     unsigned     vex_class:8;
     X86VEXSpecial vex_special:8;
     unsigned     valid_prefix:16;
+    unsigned     vex_valid_prefix:16;
     unsigned     check:16;
     unsigned     intercept:8;
     bool         has_intercept:1;
-- 
2.53.0
Re: [PATCH] target/i386: Reject VPINSRW with VEX.pp != 0b01
Posted by Andrey Polivoda 1 week ago
Testcase:
```nasm
bits 64

global _start
_start:
db 0xc5, 0xf8, 0xc4, 0x00, 0x00
int3
```
```
$ nasm -f elf64 vpinsrw_test.asm
$ ld.lld -o vpinsrw_test vpinsrw_test
```
Re: [PATCH] target/i386: Reject VPINSRW with VEX.pp != 0b01
Posted by Andrey Polivoda 6 days, 14 hours ago
Somehow messed up the commands twice...

```bash
$ nasm -f elf64 vpinsrw_test.asm
$ ld.lld -o vpinsrw_test vpinsrw_test.o
$ ./qemu-x86_64 ./vpinsrw_test
```