Re: [PATCH v3 1/2] x86/tdx: Fix off-by-one in port I/O handling

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] VMM-controlled Register Poisoning in CPUID emulation.
--

commit 8bf39531f06bffdf9286b3ddc6968088a81a8929
Author: Kiryl Shutsemau (Meta) <kas@kernel.org>

x86/tdx: Fix off-by-one in port I/O handling

This commit corrects an off-by-one error in the mask calculation for port io
handling. The previous implementation used a mask that was one bit too wide
for all io sizes, which is now corrected.

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 186915a17c50f..65119362f9a25 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -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;
>  
>  	/*

[ ... ]

[Severity: Critical]
This is a pre-existing issue, but can a malicious hypervisor poison the upper
32 bits of the registers during cpuid emulation?

Architecturally, the cpuid instruction returns 32-bit values in eax, ebx,
ecx, and edx. In 64-bit mode, writes to these 32-bit registers must
zero-extend to their 64-bit counterparts.

Looking at handle_cpuid() in arch/x86/coco/tdx/tdx.c, the 64-bit results
fetched from the untrusted hypervisor are copied directly into the guest's
pt_regs:

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;

	return ve_instr_len(ve);
}

Since these are not cast to u32 or zero-extended, does this allow the
hypervisor to supply values with non-zero upper 32 bits and potentially lead
to out-of-bounds memory accesses or control flow hijacking?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527120544.2903923-1-kas@kernel.org?part=1