Created attachment 60677 [details] Fix for the aforementioned issues I am doing some machine level cross-checking of code that has been translated through VEX and native code. In the process, I discovered several opcode bugs. I have attached a patch that fixes them and updates the tests to extend coverage over these bugs. Bug in BTC: The hack to copy a register out to the stack does not work if the rsp/esp/sp is the destination register. The implementation clobbers the result when restoring the stack pointer. The fix is to use more temporaries to store intermediate values so that the correct state can be restored. Bug in BSF/BSR: When performing dword sized bit scan operations, the upper dword of the destination register is clobbered in the case where the source dword contains no set bits. The fix is to pass the full width value of the destination register into the Mux0X operation. Bug in PUSHF: There are two bits in the RFLAGS register that should always be set. Bit 1 is a reserved bit that is always set to 1 according to the Intel documentation. Bit 9 is the interrupt enable flag which should always be set inside user mode code. Bug in CMPXCHG: The problem here is similar to the issue with BSF/BSR. The automatic extension of dwords to quadwords when storing to a register causes the upper dword of RAX or the destination register to be destroyed. The fix is to do the Mux0X operation on full 64-bit operands with the old values properly loaded as 64-bit values. These bugs all exist in 3.6.1, but I have been using VEX trunk.
I've rediscovered this problem (at least for cmpxchg) after running some QEMU differential tests. Here's a patch for recent Valgrind to fix cmpxchg behavior. I've not tried the other instructions mentioned yet. From d289e1032501fb8cf9109b7f11350d8cb326b2e5 Mon Sep 17 00:00:00 2001 From: Matt Borgerson <contact@mborgerson.com> Date: Tue, 10 Sep 2024 15:31:58 -0700 Subject: [PATCH] amd64: Fix 32b cmpxchg unmodified register writeback 32b register writes are zero-extended. In case a register should not be modified by cmpxchg, preserve the entire 64b value. --- VEX/priv/guest_amd64_toIR.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/VEX/priv/guest_amd64_toIR.c b/VEX/priv/guest_amd64_toIR.c index 57a8a434b..a55058c2e 100644 --- a/VEX/priv/guest_amd64_toIR.c +++ b/VEX/priv/guest_amd64_toIR.c @@ -8615,10 +8615,25 @@ ULong dis_cmpxchg_G_E ( /*OUT*/Bool* ok, assign( acc, getIRegRAX(size) ); setFlags_DEP1_DEP2(Iop_Sub8, acc, dest, ty); assign( cond, mk_amd64g_calculate_condition(AMD64CondZ) ); - assign( dest2, IRExpr_ITE(mkexpr(cond), mkexpr(src), mkexpr(dest)) ); - assign( acc2, IRExpr_ITE(mkexpr(cond), mkexpr(acc), mkexpr(dest)) ); - putIRegRAX(size, mkexpr(acc2)); - putIRegE(size, pfx, rm, mkexpr(dest2)); + + if (size == 4) { + /* Preserve upper 32b when register should be unmodified. */ + IRTemp acc64 = newTemp(Ity_I64); + IRTemp dest64 = newTemp(Ity_I64); + assign( dest64, IRExpr_ITE(mkexpr(cond), + unop(Iop_32Uto64, mkexpr(src)), + getIRegE(8, pfx, rm)) ); + assign( acc64, IRExpr_ITE(mkexpr(cond), + getIRegRAX(8), + unop(Iop_32Uto64, mkexpr(dest))) ); + putIRegRAX(8, mkexpr(acc64)); + putIRegE(8, pfx, rm, mkexpr(dest64)); + } else { + assign( dest2, IRExpr_ITE(mkexpr(cond), mkexpr(src), mkexpr(dest)) ); + assign( acc2, IRExpr_ITE(mkexpr(cond), mkexpr(acc), mkexpr(dest)) ); + putIRegRAX(size, mkexpr(acc2)); + putIRegE(size, pfx, rm, mkexpr(dest2)); + } DIP("cmpxchg%c %s,%s\n", nameISize(size), nameIRegG(size,pfx,rm), nameIRegE(size,pfx,rm) ); -- 2.43.0