Re: [PATCH v6 3/3] x86/tdx: Fix zero-extension for 32-bit port I/O

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] The TDX #VE handler for the CPUID instruction fails to zero the upper 32 bits of the destination registers, exposing the guest to potentially malicious or garbage data from the VMM.
- [High] The TDX #VE handler for MMIO reads fails to zero the upper 32 bits of the destination register for 32-bit MOVZX and MOVSX instructions.
- [High] The TDX #VE handler for MSR writes corrupts the MSR data sent to the VMM if the guest's RAX register contains non-zero upper 32 bits.
--

--- Patch [3]: [PATCH v6 3/3] x86/tdx: Fix zero-extension for 32-bit port I/O ---
commit 54fa9007fcfc0403e17720722b833050a2d55438
Author: Kiryl Shutsemau (Meta) <kas@kernel.org>

x86/tdx: Fix zero-extension for 32-bit port I/O

This commit updates handle_in() to use insn_assign_reg() for 32-bit port IN
instructions. This ensures proper partial-register-write semantics where
4-byte forms correctly zero-extend to the full 64-bit register.

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

> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index b8bbd715fb624..f904a636d449b 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -703,11 +703,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;
>  }

[Severity: High]
This is a pre-existing issue, but since you are fixing zero-extension issues
in this file, I noticed another zero-extension problem in handle_mmio() for
32-bit destination registers.

If a guest executes an instruction like MOVZX EAX, BYTE PTR [mem], the #VE
handler calculates extend_size:

arch/x86/coco/tdx/tdx.c:handle_mmio() {
	...
	case INSN_MMIO_READ_ZERO_EXTEND:
		/* Zero extend based on operand size */
		extend_size = insn.opnd_bytes;
		break;
	...
}

If insn.opnd_bytes is 4, doesn't this cause the subsequent memset to only
clear the lower 4 bytes of regs->ax, leaving the upper 32 bits containing
stale data and violating x86-64 architecture rules?


[Severity: High]
This isn't a bug introduced by this patch, but does handle_cpuid() have a
similar problem with zero-extending registers?

When a guest executes CPUID, it traps to the #VE handler. handle_cpuid()
copies the VMM hypercall results directly into the registers:

arch/x86/coco/tdx/tdx.c:handle_cpuid() {
	...
	regs->ax = args.r12;
	regs->bx = args.r13;
	regs->cx = args.r14;
	regs->dx = args.r15;
	...
}

Could an untrusted VMM inject non-zero data into the upper 32 bits of these
registers and violate x86-64 CPUID semantics since they are not being
zero-extended?


[Severity: High]
This is also a pre-existing issue, but could write_msr() corrupt the MSR
data sent to the VMM if the guest's RAX register contains non-zero upper
32 bits?

The payload for WRMSR is constructed like this:

arch/x86/coco/tdx/tdx.c:write_msr() {
	...
	struct tdx_module_args args = {
		...
		.r13 = (u64)regs->dx << 32 | regs->ax,
	};
	...
}

Since WRMSR ignores the upper 32 bits of RAX, the architecture permits
garbage in them. Because regs->ax is not cast to a 32-bit integer before
the bitwise-OR, won't any set bits in its upper 32 bits bleed into the
EDX portion of the payload?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713133753.223947-1-kirill@shutemov.name?part=3