x86: correctly handle XACQUIRE/XRELEASE (capstone-engine#2882)

* x86: correctly handle XACQUIRE/XRELEASE

This patch changes XACQUIRE/XRELEASE decoding to happen only after
all prefixes have been read.
This is necessary to handle all possible orderings of prefixes.
For example, both F3F0 and F0F3 should be treated as XRELEASE.
The last REP prefix is taken to distinguish between XACQUIRE and
XRELEASE. So F2F3F0 = XRELEASE, F3F2F0 = XACQUIRE.

This behavior is specified in the Intel SDM Section 6.1 -
XACQUIRE/XRELEASE.

Additionally, this patch changes the disassembly printing of
LOCKs and XACQUIRE/XRELEASE to reflect the actual prefixes used.
Previously, XCHG instructions would always be printed without LOCK
and everything else with LOCK.
For this, an extra field is added to MCInst.

* x86: add tests for REP on string instruction

Co-authored-by: Rot127 <45763064+Rot127@users.noreply.github.com>

Author: jxors

MCInst.h

@@ -133,6 +133,7 @@ struct MCInst {
 	cs_wasm_op wasm_data; // for WASM operand
 	MCRegisterInfo *MRI;
 	uint8_t xAcquireRelease; // X86 xacquire/xrelease
+	uint8_t x86Lock; // Set when the X86 LOCK prefix is present
 	bool isAliasInstr; // Flag if this MCInst is an alias.
 	bool fillDetailOps; // If set, detail->operands gets filled.
 	hppa_ext hppa_ext; ///< for HPPA operand. Contains info about modifiers and their effect on the instruction

arch/X86/X86Disassembler.c

@@ -1374,6 +1374,7 @@ bool X86_getInstruction(csh ud, const uint8_t *code, size_t code_len,
 			instr->x86_prefix[2] = insn.prefix2;
 			instr->x86_prefix[3] = insn.prefix3;
 			instr->xAcquireRelease = insn.xAcquireRelease;
+			instr->x86Lock = insn.hasLockPrefix;
 
 			if (handle->detail_opt) {
 				update_pub_insn(instr->flat_insn, &insn);

arch/X86/X86DisassemblerDecoder.c

@@ -363,13 +363,11 @@ static void setGroup0Prefix(struct InternalInstruction *insn, uint8_t prefix)
 	switch (prefix) {
 	case 0xf0: // LOCK
 		insn->hasLockPrefix = true;
-		insn->repeatPrefix = 0;
 		break;
 
 	case 0xf2: // REPNE/REPNZ
 	case 0xf3: // REP or REPE/REPZ
 		insn->repeatPrefix = prefix;
-		insn->hasLockPrefix = false;
 		break;
 	}
 }
@@ -483,31 +481,6 @@ static int readPrefixes(struct InternalInstruction *insn)
 			if (lookAtByte(insn, &nextByte))
 				return -1;
 
-			/*
-			 * If the byte is 0xf2 or 0xf3, and any of the following conditions are
-			 * met:
-			 * - it is followed by a LOCK (0xf0) prefix
-			 * - it is followed by an xchg instruction
-			 * then it should be disassembled as a xacquire/xrelease not repne/rep.
-			 */
-			if (((nextByte == 0xf0) ||
-			     ((nextByte & 0xfe) == 0x86 ||
-			      (nextByte & 0xf8) == 0x90))) {
-				insn->xAcquireRelease = byte;
-			}
-
-			/*
-			 * Also if the byte is 0xf3, and the following condition is met:
-			 * - it is followed by a "mov mem, reg" (opcode 0x88/0x89) or
-			 *                       "mov mem, imm" (opcode 0xc6/0xc7) instructions.
-			 * then it should be disassembled as an xrelease not rep.
-			 */
-			if (byte == 0xf3 &&
-			    (nextByte == 0x88 || nextByte == 0x89 ||
-			     nextByte == 0xc6 || nextByte == 0xc7)) {
-				insn->xAcquireRelease = byte;
-			}
-
 			if (isREX(insn, nextByte)) {
 				uint8_t nnextByte;
 
@@ -772,6 +745,36 @@ static int readPrefixes(struct InternalInstruction *insn)
 	} else
 		unconsumeByte(insn);
 
+	if (insn->repeatPrefix != 0) {
+		if (lookAtByte(insn, &nextByte))
+			return -1;
+
+		/*
+		* REP prefix is present, and any of the following conditions are
+		* met:
+		* - it is followed by a LOCK (0xf0) prefix
+		* - it is followed by an xchg instruction (except for 0x90 - NOP/PAUSE)
+		* then it should be disassembled as a xacquire/xrelease not repne/rep.
+		*/
+		if ((insn->hasLockPrefix || ((nextByte & 0xfe) == 0x86 ||
+					     (nextByte & 0xf8) == 0x90)) &&
+		    nextByte != 0x90) {
+			insn->xAcquireRelease = insn->repeatPrefix;
+		}
+
+		/*
+		* Also if the REP prefix is 0xf3, and the following condition is met:
+		* - it is followed by a "mov mem, reg" (opcode 0x88/0x89) or
+		*                       "mov mem, imm" (opcode 0xc6/0xc7) instructions.
+		* then it should be disassembled as an xrelease not rep.
+		*/
+		if (insn->repeatPrefix == 0xf3 &&
+		    (nextByte == 0x88 || nextByte == 0x89 || nextByte == 0xc6 ||
+		     nextByte == 0xc7)) {
+			insn->xAcquireRelease = insn->repeatPrefix;
+		}
+	}
+
 	if (insn->mode == MODE_16BIT) {
 		insn->registerSize = (insn->hasOpSize ? 4 : 2);
 		insn->addressSize = (insn->hasAdSize ? 4 : 2);

arch/X86/X86Mapping.c

@@ -1929,20 +1929,6 @@ static bool valid_bnd(cs_struct *h, unsigned int opcode)
 	// not found
 	return false;
 }
-
-// return true if the opcode is XCHG [mem]
-static bool xchg_mem(unsigned int opcode)
-{
-	switch (opcode) {
-	default:
-		return false;
-	case X86_XCHG8rm:
-	case X86_XCHG16rm:
-	case X86_XCHG32rm:
-	case X86_XCHG64rm:
-		return true;
-	}
-}
 #endif
 
 // given MCInst's id, find out if this insn is valid for REP prefix
@@ -2112,106 +2098,79 @@ bool X86_lockrep(MCInst *MI, SStream *O)
 	unsigned int opcode;
 	bool res = false;
 
-	switch (MI->x86_prefix[0]) {
+#ifndef CAPSTONE_DIET
+	switch (MI->xAcquireRelease) {
+	case 0xF2:
+		SStream_concat(O, "xacquire|");
+		break;
+	case 0xF3:
+		SStream_concat(O, "xrelease|");
+		break;
 	default:
 		break;
-	case 0xf0:
-#ifndef CAPSTONE_DIET
-		if (MI->xAcquireRelease == 0xf2)
-			SStream_concat(O, "xacquire|lock|");
-		else if (MI->xAcquireRelease == 0xf3)
-			SStream_concat(O, "xrelease|lock|");
-		else
-			SStream_concat(O, "lock|");
+	}
 #endif
-		break;
-	case 0xf2: // repne
-		opcode = MCInst_getOpcode(MI);
 
-#ifndef CAPSTONE_DIET // only care about memonic in standard (non-diet) mode
-		if (xchg_mem(opcode) && MI->xAcquireRelease) {
-			SStream_concat(O, "xacquire|");
-		} else if (valid_repne(MI->csh, opcode)) {
-			SStream_concat(O, "repne|");
-			add_cx(MI);
-		} else if (valid_bnd(MI->csh, opcode)) {
-			SStream_concat(O, "bnd|");
-		} else {
-			// invalid prefix
-			MI->x86_prefix[0] = 0;
-
-			// handle special cases
-#ifndef CAPSTONE_X86_REDUCE
-#if 0
-				if (opcode == X86_MULPDrr) {
-					MCInst_setOpcode(MI, X86_MULSDrr);
-					SStream_concat0(O, "mulsd\t");
-					res = true;
-				}
-#endif
-#endif
-		}
-#else // diet mode -> only patch opcode in special cases
-		if (!valid_repne(MI->csh, opcode)) {
-			MI->x86_prefix[0] = 0;
+	if (MI->xAcquireRelease) {
+		if (MI->x86Lock) {
+			// Force LOCK prefix as group 0 prefix for XACQUIRE and XRELEASE if a LOCK is also present.
+			// This is an arbitrary choice, since there are effectively two group 0 prefixes present.
+			// The Intel SDM is not clear on how we should interpret group 0 in this case. It states:
+			// "it is only useful to include up to one prefix code from each of the four groups"
+			// ...and then defines instructions where both an F2/F3 and F0 are useful anyway.
+			MI->x86_prefix[0] = 0xF0;
 		}
-#ifndef CAPSTONE_X86_REDUCE
-#if 0
-			// handle special cases
-			if (opcode == X86_MULPDrr) {
-				MCInst_setOpcode(MI, X86_MULSDrr);
+	} else {
+		switch (MI->x86_prefix[0]) {
+		case 0xF2:
+			opcode = MCInst_getOpcode(MI);
+#ifndef CAPSTONE_DIET
+			if (valid_repne(MI->csh, opcode)) {
+				SStream_concat(O, "repne|");
+				add_cx(MI);
+			} else if (valid_bnd(MI->csh, opcode)) {
+				SStream_concat(O, "bnd|");
+			} else {
+				// invalid prefix
+				MI->x86_prefix[0] = 0;
 			}
-#endif
-#endif
-#endif
-		break;
-
-	case 0xf3:
-		opcode = MCInst_getOpcode(MI);
-
-#ifndef CAPSTONE_DIET // only care about memonic in standard (non-diet) mode
-		if (xchg_mem(opcode) && MI->xAcquireRelease) {
-			SStream_concat(O, "xrelease|");
-		} else if (valid_rep(MI->csh, opcode)) {
-			SStream_concat(O, "rep|");
-			add_cx(MI);
-		} else if (valid_repe(MI->csh, opcode)) {
-			SStream_concat(O, "repe|");
-			add_cx(MI);
-		} else if (valid_ret_repz(MI->csh, opcode)) {
-			SStream_concat(O, "repz|");
-		} else {
-			// invalid prefix
-			MI->x86_prefix[0] = 0;
-
-			// handle special cases
-#ifndef CAPSTONE_X86_REDUCE
-#if 0
-				// FIXME: remove this special case?
-				if (opcode == X86_MULPDrr) {
-					MCInst_setOpcode(MI, X86_MULSSrr);
-					SStream_concat0(O, "mulss\t");
-					res = true;
-				}
-#endif
-#endif
-		}
-#else // diet mode -> only patch opcode in special cases
-		if (!valid_rep(MI->csh, opcode) &&
-		    !valid_repe(MI->csh, opcode)) {
-			MI->x86_prefix[0] = 0;
-		}
-#ifndef CAPSTONE_X86_REDUCE
-#if 0
-			// handle special cases
-			// FIXME: remove this special case?
-			if (opcode == X86_MULPDrr) {
-				MCInst_setOpcode(MI, X86_MULSSrr);
+#else
+			if (!valid_repne(MI->csh, opcode)) {
+				MI->x86_prefix[0] = 0;
 			}
 #endif
+			break;
+		case 0xF3:
+			opcode = MCInst_getOpcode(MI);
+#ifndef CAPSTONE_DIET
+			if (valid_rep(MI->csh, opcode)) {
+				SStream_concat(O, "rep|");
+				add_cx(MI);
+			} else if (valid_repe(MI->csh, opcode)) {
+				SStream_concat(O, "repe|");
+				add_cx(MI);
+			} else if (valid_ret_repz(MI->csh, opcode)) {
+				SStream_concat(O, "repz|");
+			} else {
+				// invalid prefix
+				MI->x86_prefix[0] = 0;
+			}
+#else
+			if (!valid_rep(MI->csh, opcode) &&
+			    !valid_repe(MI->csh, opcode)) {
+				MI->x86_prefix[0] = 0;
+			}
 #endif
-#endif
-		break;
+			break;
+		default:
+			break;
+		}
+	}
+
+	// LOCK and F2/F3 may both be present (for XACQUIRE/XRELEASE).
+	// There are also XRELEASEs that can be LOCKless.
+	if (MI->x86Lock) {
+		SStream_concat(O, "lock|");
 	}
 
 	switch (MI->x86_prefix[1]) {