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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] In `handle_mmio()`, the `val` variable is not fully initialized before being sent to the VMM during a memory-mapped I/O write emulation, leading to a kernel stack memory leak.
- [High] The CPUID instruction emulation in `handle_cpuid()` fails to zero-extend the 32-bit results into their respective 64-bit registers.
- [High] The MMIO emulation for MOVZX and MOVSX instructions in `handle_mmio()` fails to clear the upper 32 bits when the destination is a 32-bit register.
- [High] In `write_msr()`, `regs->ax` is not masked to 32 bits before being ORed to form the MSR value, which can corrupt the upper 32 bits (representing EDX) sent to the VMM.
--

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

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

According to x86 architecture rules, 32-bit operations zero-extend the
result to 64 bits. Update handle_in() to zero out the entire RAX register
when the I/O size is 4 bytes to ensure correct zero-extension.

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 65119362f9a25..58feca4193269 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c

[ ... ]

> static int write_msr(struct pt_regs *regs, struct ve_info *ve)
> {
> 	struct tdx_module_args args = {
> 		.r10 = TDX_HYPERCALL_STANDARD,
> 		.r11 = hcall_func(EXIT_REASON_MSR_WRITE),
> 		.r12 = regs->cx,
> 		.r13 = (u64)regs->dx << 32 | regs->ax,
        ^^^^^^^^

[Severity: High]
This isn't a bug introduced by this patch, but does this correctly build the
MSR value?

Since regs->ax is a 64-bit value, it looks like any non-zero bits in the
upper 32 bits of regs->ax will not be cleared.

Will these upper bits then be logically ORed into the upper 32 bits of the
result, corrupting the EDX portion of the WRMSR value sent to the VMM?

Could a guest program with legitimately non-zero data in the upper 32 bits of
RAX prior to the #VE inadvertently write wrong values to MSRs?

[ ... ]

> static int handle_cpuid(struct pt_regs *regs, struct ve_info *ve)
> {
> 	struct tdx_module_args args = {
> 		.r10 = TDX_HYPERCALL_STANDARD,
> 		.r11 = hcall_func(EXIT_REASON_CPUID),
> 		.r12 = regs->ax,
> 		.r13 = regs->cx,
> 	};

[ ... ]

> 	/*
> 	 * 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;
        ^^^^^^^^

[Severity: High]
This is a pre-existing issue, but does this correctly handle zero-extension
for 32-bit results into 64-bit registers?

According to x86-64 architecture rules, writes to 32-bit registers must clear
the upper 32 bits of the corresponding 64-bit registers.

Because the values are copied directly from the untrusted VMM (args.r12 to
args.r15) into the registers without masking to 32 bits, could a malicious
VMM inject non-zero values into the upper 32 bits?

Can this corrupt guest user-space or kernel state if the code relies on these
bits being zeroed?

[ ... ]

> static int handle_mmio(struct pt_regs *regs, struct ve_info *ve)
> {
> 	unsigned long *reg, val, vaddr;
                            ^^^

[Severity: Critical]
This is a pre-existing issue, but is val fully initialized before being sent
to the VMM?

When handling an MMIO write of size smaller than 8 bytes, does memcpy only
write to the lowest size bytes, leaving the upper bytes of val uninitialized
with kernel stack data?

[ ... ]

> 	/* Handle writes first */
> 	switch (mmio) {
> 	case INSN_MMIO_WRITE:
> 		memcpy(&val, reg, size);
> 		if (!mmio_write(size, ve->gpa, val))

Because this 8-byte variable is passed to mmio_write() and forwarded to the
untrusted VMM, does this leak uninitialized kernel stack memory to the guest
hypervisor?

[ ... ]

> 	switch (mmio) {
> 	case INSN_MMIO_READ:
> 		/* Zero-extend for 32-bit operation */
> 		extend_size = size == 4 ? sizeof(*reg) : 0;
> 		break;
> 	case INSN_MMIO_READ_ZERO_EXTEND:
> 		/* Zero extend based on operand size */
> 		extend_size = insn.opnd_bytes;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This isn't a bug introduced by this patch, but does this properly clear the
upper 32 bits when the destination is a 32-bit register?

For MOVZX and MOVSX instructions, extend_size is set to insn.opnd_bytes.
When the destination is a 32-bit register, insn.opnd_bytes is 4.

Does memset then only clear or set 4 bytes of the 64-bit register?

Because x86-64 architecture dictates that writing to a 32-bit register always
zero-extends to the full 64-bit register, could compiler-generated code
relying on this zero-extension use garbage values left over in the upper
half, leading to functional regressions or out-of-bounds memory accesses?

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