x86/tdx: Fix port I/O handling bugs

handle_in() and handle_out() in arch/x86/coco/tdx/tdx.c use:

    u64 mask = GENMASK(BITS_PER_BYTE * size, 0);

GENMASK(h, l) includes bit h. For size=1 (INB), this produces GENMASK(8, 0) = 0x1FF (9 bits) instead of GENMASK(7, 0) = 0xFF (8 bits). The mask is one bit too wide for all I/O sizes.

Fix the mask calculation.

Fixes 03149948832a ("x86/tdx: Port I/O: Add runtime hypercalls")

Reported-by Borys Tsyrulnikov <[email protected]>

Link https://lore.kernel.org/all/CAKw_Dz96rfSQc6Rn+9QBcUFHhmkK+9zu+P=bxowfZwxrATCBRg@mail.gmail.com/ open here

Signed-off-by Kiryl Shutsemau (Meta) <[email protected]>

Reviewed-by Kai Huang <[email protected]>

Reviewed-by Kuppuswamy Sathyanarayanan <[email protected]>

Cc [email protected]

---
 arch/x86/coco/tdx/tdx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/x86/coco/tdx/tdx.c +2 -2

index 186915a17c50..65119362f9a2 100644
@@ -693,7 +693,7 @@ static bool handle_in(struct pt_regs *regs, int size, int port)
 		.r13 = PORT_READ,
 		.r14 = port,
 	};
-	u64 mask = GENMASK(BITS_PER_BYTE * size, 0);
+	u64 mask = GENMASK(BITS_PER_BYTE * size - 1, 0);
 	bool success;
 
 	/*

@@ -713,7 +713,7 @@ static bool handle_in(struct pt_regs *regs, int size, int port)
 
 static bool handle_out(struct pt_regs *regs, int size, int port)
 {
-	u64 mask = GENMASK(BITS_PER_BYTE * size, 0);
+	u64 mask = GENMASK(BITS_PER_BYTE * size - 1, 0);
 
 	/*
 	 * Emulate the I/O write via hypercall. More info about ABI can be found

According to x86 architecture rules, 32-bit operations zero-extend the result to 64 bits. The current implementation of handle_in() only masks the lower 32 bits, which preserves the upper 32 bits of RAX when a 32-bit port IN instruction is emulated.

Use insn_assign_reg() to write the result back into RAX with proper partial-register-write semantics: 1- and 2-byte forms leave the upper bits untouched, the 4-byte form zero-extends to the full register.

Fixes 03149948832a ("x86/tdx: Port I/O: Add runtime hypercalls")

Reported-by Borys Tsyrulnikov <[email protected]>

Link https://lore.kernel.org/all/CAKw_Dz96rfSQc6Rn+9QBcUFHhmkK+9zu+P=bxowfZwxrATCBRg@mail.gmail.com/ open here

Signed-off-by Kiryl Shutsemau <[email protected]>

Cc [email protected]

---
 arch/x86/coco/tdx/tdx.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

arch/x86/coco/tdx/tdx.c +3 -5

index 65119362f9a2..41cc23cc63dd 100644
@@ -693,8 +693,8 @@ static bool handle_in(struct pt_regs *regs, int size, int port)
 		.r13 = PORT_READ,
 		.r14 = port,
 	};
-	u64 mask = GENMASK(BITS_PER_BYTE * size - 1, 0);
 	bool success;
+	u64 val;
 
 	/*
 	 * Emulate the I/O read via hypercall. More info about ABI can be found

@@ -702,11 +702,9 @@ static bool handle_in(struct pt_regs *regs, int size, int port)
 	 * "TDG.VP.VMCALL<Instruction.IO>".
 	 */
 	success = !__tdx_hypercall(&args);
+	val = success ? args.r11 : 0;
 
-	/* Update part of the register affected by the emulated instruction */
-	regs->ax &= ~mask;
-	if (success)
-		regs->ax |= args.r11 & mask;
+	insn_assign_reg(&regs->ax, val, size);
 
 	return success;
 }

Quoted 9 lines · Kiryl Shutsemau (Meta)

According to x86 architecture rules, 32-bit operations zero-extend the result to 64 bits. The current implementation of handle_in() only masks the lower 32 bits, which preserves the upper 32 bits of RAX when a 32-bit port IN instruction is emulated.

Use insn_assign_reg() to write the result back into RAX with proper partial-register-write semantics: 1- and 2-byte forms leave the upper bits untouched, the 4-byte form zero-extends to the full register.

Quoted 5 lines

Fixes 03149948832a ("x86/tdx: Port I/O: Add runtime hypercalls")

Reported-by Borys Tsyrulnikov <[email protected]>

Link https://lore.kernel.org/all/CAKw_Dz96rfSQc6Rn+9QBcUFHhmkK+9zu+P=bxowfZwxrATCBRg@mail.gmail.com/ open here

Signed-off-by Kiryl Shutsemau <[email protected]>

Cc [email protected]

I think the concern sashiko commented in patch 2 is valid.

KVM's instruction emulator has a small helper, assign_register(), that writes a value into a sub-register with x86 partial-register-write
semantics: 1- and 2-byte writes leave the upper bits of the destination untouched, 4-byte writes zero-extend to 64 bits, 8-byte writes overwrite the full register.

The TDX guest #VE handler needs the same logic for port I/O emulation to get 32-bit zero-extension right. Rather than copy-pasting the helper, lift it to <asm/insn-eval.h> as insn_assign_reg() so both can use it.

Rewrite the body using arithmetic instead of pointer punning so the helper does not depend on -fno-strict-aliasing or little-endian byte order, and add <asm/insn.h> to the header's includes so it builds standalone in callers that have not pulled it in transitively.

No functional change.

Signed-off-by Kiryl Shutsemau <[email protected]>

handle_in() and handle_out() in arch/x86/coco/tdx/tdx.c use:

    u64 mask = GENMASK(BITS_PER_BYTE * size, 0);

GENMASK(h, l) includes bit h. For size=1 (INB), this produces GENMASK(8, 0) = 0x1FF (9 bits) instead of GENMASK(7, 0) = 0xFF (8 bits). The mask is one bit too wide for all I/O sizes.

Fix the mask calculation.

Fixes 03149948832a ("x86/tdx: Port I/O: Add runtime hypercalls")

Reported-by Borys Tsyrulnikov <[email protected]>

Link https://lore.kernel.org/all/CAKw_Dz96rfSQc6Rn+9QBcUFHhmkK+9zu+P=bxowfZwxrATCBRg@mail.gmail.com/ open here

Signed-off-by Kiryl Shutsemau (Meta) <[email protected]>

Reviewed-by Kai Huang <[email protected]>

Reviewed-by Kuppuswamy Sathyanarayanan <[email protected]>

Cc [email protected]

We should probably ping Sean/Paolo to ack this if we want Dave to take it. Is it the plan?

KVM's instruction emulator has a small helper, assign_register(), that writes a value into a sub-register with x86 partial-register-write
semantics: 1- and 2-byte writes leave the upper bits of the destination untouched, 4-byte writes zero-extend to 64 bits, 8-byte writes overwrite the full register.

The TDX guest #VE handler needs the same logic for port I/O emulation to get 32-bit zero-extension right. Rather than copy-pasting the helper, lift it to <asm/insn-eval.h> as insn_assign_reg() so both can use it.

Rewrite the body using arithmetic instead of pointer punning so the helper does not depend on -fno-strict-aliasing

or little-endian byte order,

and add <asm/insn.h> to the header's includes so it builds standalone in callers that have not pulled it in transitively.

No functional change.

Signed-off-by Kiryl Shutsemau <[email protected]>

---
 arch/x86/include/asm/insn-eval.h | 25 +++++++++++++++++++++++++
 arch/x86/kvm/emulate.c           | 26 ++++----------------------
 2 files changed, 29 insertions(+), 22 deletions(-)

arch/x86/include/asm/insn-eval.h +1 -0

index 4733e9064ee5..85251e718a77 100644
@@ -9,6 +9,7 @@
 #include <linux/compiler.h>
 #include <linux/bug.h>
 #include <linux/err.h>
+#include <asm/insn.h>
 #include <asm/ptrace.h>
 
 #define INSN_CODE_SEG_ADDR_SZ(params) ((params >> 4) & 0xf)

arch/x86/include/asm/insn-eval.h +12 -0

+		break;
+	case 2:
+		*reg = (*reg & ~0xfffful) | (val & 0xffff);
+		break;
+	case 4:
+		*reg = (u32)val;
+		break;
+	case 8:
+		*reg = val;
+		break;
+	}
+}