Re: [PATCH v4 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 1 potential issue(s) to consider:

Pre-existing issues:
- [High] handle_cpuid() fails to clear the upper 32 bits of RAX, RBX, RCX, RDX, allowing an untrusted VMM to inject garbage into guest registers.
--

commit 1a8c131a02100639d5d76e680c87ef32990b4663
Author: Kiryl Shutsemau (Meta) <kas@kernel.org>

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

This commit fixes the zero-extension behavior for emulated 32-bit port I/O in
handle_in(). It uses insn_assign_reg() to correctly write the result into RAX
with proper partial-register-write semantics, ensuring the upper 32 bits are
zeroed for 4-byte reads.

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 4d7f71d50122e..fcb9ec936d050 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -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);

[Severity: High]
This isn't a bug introduced by this patch, but while you are fixing the
zero-extension semantics for handle_in(), could the same issue be present
in handle_cpuid() when emulating CPUID?

In arch/x86/coco/tdx/tdx.c:handle_cpuid():

	/*
	 * As per TDX GHCI CPUID ABI, r12-r15 registers contain contents of
	 * EAX, EBX, ECX, EDX registers after the CPUID instruction execution.
	 * So copy the register contents back to pt_regs.
	 */
	regs->ax = args.r12;
	regs->bx = args.r13;
	regs->cx = args.r14;
	regs->dx = args.r15;

Does this code fail to zero out the upper 32 bits of RAX, RBX, RCX, and RDX
in 64-bit mode?

Since CPUID should clear the upper 32 bits of these registers, does directly
assigning the 64-bit values from the untrusted hypervisor allow a malicious
VMM to inject non-zero upper bits into the guest registers?

If guest software relies on the upper 32 bits being zeroed after CPUID,
could this lead to unexpected behavior or bounds-check bypasses in 64-bit
contexts?

>  
>  	return success;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1780584300.git.kas@kernel.org?part=3