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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Emulated 32-bit I/O reads (inl) fail to zero-extend the result into the 64-bit RAX register, violating x86-64 semantics.
- [High] Guest register state is corrupted on the error/fault path of an I/O read.
--

--- Patch [1]: [PATCH v6 1/3] x86/tdx: Fix off-by-one in port I/O handling ---
commit 6a0c705281bac273b6793004a9d727acc5a4f92d
Author: Kiryl Shutsemau (Meta) <kas@kernel.org>

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

handle_in() and handle_out() in arch/x86/coco/tdx/tdx.c use a mask that
is one bit too wide for all I/O sizes. For size=1 (INB), this produces
GENMASK(8, 0) = 0x1FF (9 bits) instead of GENMASK(7, 0) = 0xFF (8 bits).
This commit fixes the mask calculation.

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 29b6f1ed59ec5..b8bbd715fb624 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -694,7 +694,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);

[Severity: High]
This isn't a bug introduced by this patch, but does the bitwise operation in
arch/x86/coco/tdx/tdx.c:handle_in() properly zero-extend a 32-bit read?

When a guest executes a 32-bit in instruction (inl) triggering handle_in()
with size == 4, mask is evaluated to 32 bits (0xFFFFFFFF).

arch/x86/coco/tdx/tdx.c:handle_in() {
    ...
	success = !__tdx_hypercall(&args);

	/* Update part of the register affected by the emulated instruction */
	regs->ax &= ~mask;
	if (success)
		regs->ax |= args.r11 & mask;
    ...
}

The bitwise operation regs->ax &= ~mask will clear the lower 32 bits, but it
incorrectly preserves the upper 32 bits of the 64-bit register. x86-64
architectural semantics require any 32-bit write to a general-purpose
register to zero-extend to 64 bits.

Could this cause data corruption for guest software expecting RAX to be
zero-extended?

[Severity: High]
This is another pre-existing issue, but is it safe to unconditionally clear
the target bits in regs->ax on the error path?

If the VMM rejects the hypercall (__tdx_hypercall() returns a non-zero
error), handle_in() still performs regs->ax &= ~mask; and returns false.
The kernel then injects a #GP fault into the guest.

The guest exception handler will observe a partially modified RAX register
instead of the unmodified state from before the faulting instruction. 

Can this violate architectural guarantees for faults by exposing corrupted
register state?

>  	bool success;
>  
>  	/*
[ ... ]

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