[PATCH 0/1] Fix tcg assert when decoding "lock cmp"

[PATCH 0/1] Fix tcg assert when decoding "lock cmp"

[Ziqiao Kong]

(Resending again because I found I accidentally unsubscribed the list. And
sorry for disturbance for Richard.)

Hello QEMU Developers! I'm from the downstream project Unicorn
Engine (https://github.com/unicorn-engine/unicorn). I identified a simple
bug that triggers a tcg assertion in QEMU. Although I used a usermode
usecase to illustrate the bug, it shall also affect system emulation.

Assume a binary looks like:

```
08049845 <main>:
 8049845:	55                   	push   %ebp
 8049846:	89 e5                	mov    %esp,%ebp
 8049848:	e8 0e 00 00 00       	call   804985b <__x86.get_pc_thunk.ax>
 804984d:	90                   	nop
 804984e:	90                   	nop
 804984f:	90                   	nop
 8049850:	90                   	nop
 8049851:	90                   	nop
 8049852:	f0 38 b8 00 00 00 00 	lock cmp %bh,0x0(%eax)
 8049859:	5d                   	pop    %ebp
 804985a:	c3                   	ret
```

Executing this with

```
./i386-linux-user/qemu-i386 -d op,op_opt ./tt 2>&1 | tail -100
```

gives:

```
OP:
...
...
 ---- 0000000008049852 0000000000000000
 mov_i32 loc2,eax
 extract_i32 loc1,ebx,$0x8,$0x8
 mov_i32 cc_src,loc1
 mov_i32 loc8,loc0 // loc0 undefined!
...
...
OP after optimization and liveness analysis:
...
...
 ---- 0000000008049852 0000000000000000
 extract_i32 tmp1,ebx,$0x8,$0x8           dead: 1  pref=0xffff
 mov_i32 cc_src,tmp1                      sync: 0  dead: 1  pref=0xffff
 sub_i32 cc_dst,tmp0,cc_src               sync: 0  dead: 0 1 2  pref=0xffff // so does tmp0!
...
...
ERROR:../tcg/tcg.c:4446:temp_load: code should not be reached
Bail out! ERROR:../tcg/tcg.c:4446:temp_load: code should not be reached
```

This results an assertion error because when decoding "lock cmp %bh,0x0(%eax)",
s->T0 is not properly initialized in gen_op. However, the root cause is that
this instruction is illegal refering to the Intel manual. Therefore, I add
and extra check to generate corresponding illegal opcode exception.

Signed-off-by: Ziqiao Kong <ziqiaokong@gmail.com>

Ziqiao Kong (1):
  Generate an illegal opcode exception on cmp instructions with lock
    prefix

 target/i386/tcg/translate.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

-- 
2.40.1

[PATCH 1/1] target/i386: Generate an illegal opcode exception on cmp instructions with lock prefix

[Ziqiao Kong]

target/i386: As specified by Intel Manual Vol2 3-180, cmp instructions
are not allowed to have lock prefix and a `UD` should be raised. Without
this patch, s1->T0 will be uninitialized and used in the case OP_CMPL.

Signed-off-by: Ziqiao Kong <ziqiaokong@gmail.com>
---
 target/i386/tcg/translate.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 10cba16256..07f642dc9e 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -1507,12 +1507,13 @@ static bool check_iopl(DisasContext *s)
 /* if d == OR_TMP0, it means memory operand (address in A0) */
 static void gen_op(DisasContext *s1, int op, MemOp ot, int d)
 {
+    /* Invalid lock prefix when destination is not memory or OP_CMPL. */
+    if ((d != OR_TMP0 || op == OP_CMPL) && s1->prefix & PREFIX_LOCK) {
+        gen_illegal_opcode(s1);
+        return;
+    }
+
     if (d != OR_TMP0) {
-        if (s1->prefix & PREFIX_LOCK) {
-            /* Lock prefix when destination is not memory.  */
-            gen_illegal_opcode(s1);
-            return;
-        }
         gen_op_mov_v_reg(s1, ot, s1->T0, d);
     } else if (!(s1->prefix & PREFIX_LOCK)) {
         gen_op_ld_v(s1, ot, s1->T0, s1->A0);
-- 
2.40.1

Re: [PATCH 1/1] target/i386: Generate an illegal opcode exception on cmp instructions with lock prefix

[Paolo Bonzini]

On 2/15/24 10:50, Ziqiao Kong wrote:
> target/i386: As specified by Intel Manual Vol2 3-180, cmp instructions
> are not allowed to have lock prefix and a `UD` should be raised. Without
> this patch, s1->T0 will be uninitialized and used in the case OP_CMPL.
> 
> Signed-off-by: Ziqiao Kong <ziqiaokong@gmail.com>
> ---
>   target/i386/tcg/translate.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)

Queued, thanks!

Paolo

> diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
> index 10cba16256..07f642dc9e 100644
> --- a/target/i386/tcg/translate.c
> +++ b/target/i386/tcg/translate.c
> @@ -1507,12 +1507,13 @@ static bool check_iopl(DisasContext *s)
>   /* if d == OR_TMP0, it means memory operand (address in A0) */
>   static void gen_op(DisasContext *s1, int op, MemOp ot, int d)
>   {
> +    /* Invalid lock prefix when destination is not memory or OP_CMPL. */
> +    if ((d != OR_TMP0 || op == OP_CMPL) && s1->prefix & PREFIX_LOCK) {
> +        gen_illegal_opcode(s1);
> +        return;
> +    }
> +
>       if (d != OR_TMP0) {
> -        if (s1->prefix & PREFIX_LOCK) {
> -            /* Lock prefix when destination is not memory.  */
> -            gen_illegal_opcode(s1);
> -            return;
> -        }
>           gen_op_mov_v_reg(s1, ot, s1->T0, d);
>       } else if (!(s1->prefix & PREFIX_LOCK)) {
>           gen_op_ld_v(s1, ot, s1->T0, s1->A0);

[PATCH 0/1] Fix tcg assert when decoding "lock cmp"

[Ziqiao Kong]

(Resending this series since it looks like being ignored by the list)

Hello QEMU Developers! I'm from the downstream project Unicorn
Engine (https://github.com/unicorn-engine/unicorn). I identified a simple
bug that triggers a tcg assertion in QEMU. Although I used a usermode
usecase to illustrate the bug, it shall also affect system emulation.

Assume a binary looks like:

```
08049845 <main>:
 8049845:	55                   	push   %ebp
 8049846:	89 e5                	mov    %esp,%ebp
 8049848:	e8 0e 00 00 00       	call   804985b <__x86.get_pc_thunk.ax>
 804984d:	90                   	nop
 804984e:	90                   	nop
 804984f:	90                   	nop
 8049850:	90                   	nop
 8049851:	90                   	nop
 8049852:	f0 38 b8 00 00 00 00 	lock cmp %bh,0x0(%eax)
 8049859:	5d                   	pop    %ebp
 804985a:	c3                   	ret
```

Executing this with

```
./i386-linux-user/qemu-i386 -d op,op_opt ./tt 2>&1 | tail -100
```

gives:

```
OP:
...
...
 ---- 0000000008049852 0000000000000000
 mov_i32 loc2,eax
 extract_i32 loc1,ebx,$0x8,$0x8
 mov_i32 cc_src,loc1
 mov_i32 loc8,loc0 // loc0 undefined!
...
...
OP after optimization and liveness analysis:
...
...
 ---- 0000000008049852 0000000000000000
 extract_i32 tmp1,ebx,$0x8,$0x8           dead: 1  pref=0xffff
 mov_i32 cc_src,tmp1                      sync: 0  dead: 1  pref=0xffff
 sub_i32 cc_dst,tmp0,cc_src               sync: 0  dead: 0 1 2  pref=0xffff // so does tmp0!
...
...
ERROR:../tcg/tcg.c:4446:temp_load: code should not be reached
Bail out! ERROR:../tcg/tcg.c:4446:temp_load: code should not be reached
```

This results an assertion error because when decoding "lock cmp %bh,0x0(%eax)",
s->T0 is not properly initialized in gen_op. However, the root cause is that
this instruction is illegal refering to the Intel manual. Therefore, I add
and extra check to generate corresponding illegal opcode exception.

Signed-off-by: Ziqiao Kong <ziqiaokong@gmail.com>

Ziqiao Kong (1):
  Generate an illegal opcode exception on cmp instructions with lock
    prefix

 target/i386/tcg/translate.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

-- 
2.40.1

[PATCH 1/1] target/i386: Generate an illegal opcode exception on cmp instructions with lock prefix

[Ziqiao Kong]

target/i386: As specified by Intel Manual Vol2 3-180, cmp instructions
are not allowed to have lock prefix and a `UD` should be raised. Without
this patch, s1->T0 will be uninitialized and used in the case OP_CMPL.

Signed-off-by: Ziqiao Kong <ziqiaokong@gmail.com>
---
 target/i386/tcg/translate.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 10cba16256..07f642dc9e 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -1507,12 +1507,13 @@ static bool check_iopl(DisasContext *s)
 /* if d == OR_TMP0, it means memory operand (address in A0) */
 static void gen_op(DisasContext *s1, int op, MemOp ot, int d)
 {
+    /* Invalid lock prefix when destination is not memory or OP_CMPL. */
+    if ((d != OR_TMP0 || op == OP_CMPL) && s1->prefix & PREFIX_LOCK) {
+        gen_illegal_opcode(s1);
+        return;
+    }
+
     if (d != OR_TMP0) {
-        if (s1->prefix & PREFIX_LOCK) {
-            /* Lock prefix when destination is not memory.  */
-            gen_illegal_opcode(s1);
-            return;
-        }
         gen_op_mov_v_reg(s1, ot, s1->T0, d);
     } else if (!(s1->prefix & PREFIX_LOCK)) {
         gen_op_ld_v(s1, ot, s1->T0, s1->A0);
-- 
2.40.1