SUMMARY When running valgrind on code that was assembled by a JIT, the assembly combination of a REX.W prefix followed by POPF (0x9D) fails an assertion: > vex: priv/guest_amd64_toIR.c:20628 (dis_ESC_NONE): Assertion `sz == 2 || sz == 4' failed. which comes from this section of `dis_ESC_NONE`: ``` case 0x9D: /* POPF */ /* Note. There is no encoding for a 32-bit popf in 64-bit mode. So sz==4 actually means sz==8. */ if (haveF2orF3(pfx)) goto decode_failure; vassert(sz == 2 || sz == 4); if (sz == 4) sz = 8; if (sz != 8) goto decode_failure; // until we know a sz==2 test case exists ``` The input parameter `sz` is set to 8 in `disInstr_AMD64_WRK` by this line: > if ((pfx & PFX_REX) && (pfx & PFX_REXW)) sz = 8; To my knowledge, REX.W+POPF is a valid instruction combination and should be allowed. STEPS TO REPRODUCE 1. have assembly instructions 0x48 0x9D as REX.W+POPF 2. run under valgrind OBSERVED RESULT > vex: priv/guest_amd64_toIR.c:20628 (dis_ESC_NONE): Assertion `sz == 2 || sz == 4' failed. followed by program exit. EXPECTED RESULT This should be allowed and execution should continue. ADDITIONAL INFORMATION The following patch seems to fix this: ``` diff --git a/VEX/priv/guest_amd64_toIR.c b/VEX/priv/guest_amd64_toIR.c index 7a20d45..21a3a6f 100644 --- a/VEX/priv/guest_amd64_toIR.c +++ b/VEX/priv/guest_amd64_toIR.c @@ -20625,7 +20625,7 @@ Long dis_ESC_NONE ( /* Note. There is no encoding for a 32-bit popf in 64-bit mode. So sz==4 actually means sz==8. */ if (haveF2orF3(pfx)) goto decode_failure; - vassert(sz == 2 || sz == 4); + vassert(sz == 2 || sz == 4 || sz == 8); if (sz == 4) sz = 8; if (sz != 8) goto decode_failure; // until we know a sz==2 test case exists t1 = newTemp(Ity_I64); t2 = newTemp(Ity_I64); ```
We had something similar in bug #422174 with REX prefixed JMP instructions. Lets see what the instruction manual says...
(In reply to Mark Wielaard from comment #1) > We had something similar in bug #422174 with REX prefixed JMP instructions. > Lets see what the instruction manual says... It says (for PUSH, POP, PUSHF/D/Q, POPF/D/Q and LEAVE): "When in 64-bit mode, instruction defaults to 64-bit operand size and cannot encode 32-bit operand size." Which is explains the sz == to sz = 8 "upgrade" we always do. It seems a REX.W prefix simply explicitly sets the operant size to 8, and so can/must be ignored as redundant.
I checked. PUSH and POP do always upgrade sz == 4 to sz = 8 but allow sz == 8 too. PUSHF actually has a note about having seen a dedundant REX prefix: case 0x9C: /* PUSHF */ { /* Note. There is no encoding for a 32-bit pushf in 64-bit mode. So sz==4 actually means sz==8. */ /* 24 July 06: has also been seen with a redundant REX prefix, so must also allow sz==8. */ We don't handle sz == 8 for LEAVE. But reading the instruction manual that seems tricky since it has to match the size operand of the corresponding ENTER (where we also don't handle sz == 8). In summary, I think the patch is correct.
commit e2e830f61271c28dddfa6b478044870d2188cf57 Author: Mark Wielaard <mark@klomp.org> Date: Fri Feb 12 20:42:00 2021 +0100 PR432809 VEX should support REX.W + POPF It seems a REX.W prefix simply explicitly sets the operant size to 8, and so can/must be ignored as redundant. This is what we already do for PUSH, POP and PUSHF. All instructions are described as "When in 64-bit mode, instruction defaults to 64-bit operand size and cannot encode 32-bit operand size." in the instruction manual. Original patch and analysis by Mike Dalessio <mike.dalessio@gmail.com> https://bugs.kde.org/show_bug.cgi?id=432809
Thank you for the quick turnaround!