Re: [PATCH v3 2/2] KVM: x86: Add support for cmpxchg16b emulation

[sashiko-bot@kernel.org]

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

New issues:
- [High] Emulation of CMPXCHG16B lacks the mandatory 16-byte alignment check, leading to a guest-triggerable VM crash.
- [Low] The macro `em_cmpxchg8b_16b` incorrectly captures the variable `ctxt` from the surrounding scope instead of solely using its argument `__c`.

Pre-existing issues:
- [Critical] TOCTOU vulnerability in atomic CMPXCHG emulation leads to silent data corruption in the guest when a native compare fails.
--

commit f6a8a67f38b03c4522f119ac54d63d97691e1289
Author: Sairaj Kodilkar <sarunkod@amd.com>

KVM: x86: Add support for cmpxchg16b emulation

This patch adds support for emulating cmpxchg16b when the destination memory
is 128-bit and REX.W = 1. This prevents QEMU guests from hitting emulation
failures during 256-bit device table entries accesses on AMD vIOMMU.

> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index c1b21282187f3..f38c4f1a99c31 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c

[ ... ]

> @@ -2184,24 +2184,36 @@ static int em_call_near_abs(struct x86_emulate_ctxt *ctxt)
>  	return rc;
>  }
>  
> +#define em_cmpxchg8b_16b(__c, rbits, mbits)\
> +do {												\
> +	u##mbits old = __c->dst.orig_val##mbits;						\
> +												\
> +	BUILD_BUG_ON(rbits * 2 != mbits);							\
> +												\
> +	if (((u##rbits) (old >> 0) != (u##rbits) reg_read(ctxt, VCPU_REGS_RAX)) ||		\

[Severity: Low]
Does this macro intentionally reference the unpassed variable ctxt instead of
the provided argument __c?

This uses ctxt to read registers, which makes the macro rely on the specific
variable name ctxt in the caller's scope instead of its parameter.

> +	    ((u##rbits) (old >> rbits) != (u##rbits) reg_read(ctxt, VCPU_REGS_RDX))) {		\
> +		*reg_write(ctxt, VCPU_REGS_RAX) = (u##rbits) (old >> 0);			\
> +		*reg_write(ctxt, VCPU_REGS_RDX) = (u##rbits) (old >> rbits);			\
> +		ctxt->eflags &= ~X86_EFLAGS_ZF;							\
> +	} else {										\
> +		ctxt->dst.val##mbits = ((u##mbits)reg_read(ctxt, VCPU_REGS_RCX) << rbits) |	\
> +					(u##rbits) reg_read(ctxt, VCPU_REGS_RBX);		\
> +												\
> +		ctxt->eflags |= X86_EFLAGS_ZF;							\
> +	}											\
> +} while(0)
> +
>  static int em_cmpxchg8b(struct x86_emulate_ctxt *ctxt)
>  {
> -	u64 old = ctxt->dst.orig_val64;
> -
> -	if (ctxt->dst.bytes == 16)
> +	if (WARN_ON_ONCE(8 + !!(ctxt->rex_bits & REX_W) * 8 != ctxt->dst.bytes))
>  		return X86EMUL_UNHANDLEABLE;
>  
> -	if (((u32) (old >> 0) != (u32) reg_read(ctxt, VCPU_REGS_RAX)) ||
> -	    ((u32) (old >> 32) != (u32) reg_read(ctxt, VCPU_REGS_RDX))) {
> -		*reg_write(ctxt, VCPU_REGS_RAX) = (u32) (old >> 0);
> -		*reg_write(ctxt, VCPU_REGS_RDX) = (u32) (old >> 32);
> -		ctxt->eflags &= ~X86_EFLAGS_ZF;
> -	} else {
> -		ctxt->dst.val64 = ((u64)reg_read(ctxt, VCPU_REGS_RCX) << 32) |
> -			(u32) reg_read(ctxt, VCPU_REGS_RBX);
> -
> -		ctxt->eflags |= X86_EFLAGS_ZF;
> -	}
> +	if (!(ctxt->rex_bits & REX_W))
> +		em_cmpxchg8b_16b(ctxt, 32, 64);
> +#ifdef CONFIG_X86_64
> +	else
> +		em_cmpxchg8b_16b(ctxt, 64, 128);
> +#endif

[Severity: High]
Are we missing a 16-byte alignment check for the destination memory when
executing CMPXCHG16B here?

The x86 architecture strictly mandates that the memory operand for CMPXCHG16B
must be 16-byte aligned. If it is not, a #GP(0) exception is generated
unconditionally.

Without this check, KVM will later attempt a native unaligned lock cmpxchg16b,
which will raise a host #GP(0). The host exception table catches this and
returns -EFAULT, translating to X86EMUL_UNHANDLEABLE, which immediately
crashes the VM via KVM_EXIT_INTERNAL_ERROR. Can this regression be used by a
guest to trigger a denial of service?

>  	return X86EMUL_CONTINUE;
>  }

[ ... ]

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 79468ddfe4736..60be16f05d794 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> @@ -5311,6 +5311,11 @@ static int emulator_cmpxchg_emulated(struct x86_emulate_ctxt *ctxt,
>  	case 8:
>  		r = emulator_try_cmpxchg_user(u64, hva, old, new);
>  		break;
> +#ifdef CONFIG_X86_64
> +	case 16:
> +		r = emulator_try_cmpxchg_user(u128, hva, old, new);
> +		break;
> +#endif

[Severity: Critical]
This is a pre-existing issue, but does the emulation of atomic CMPXCHG suffer
from a TOCTOU race leading to silent data corruption in the guest?

When emulator_try_cmpxchg_user() fails due to concurrent memory modification,
emulator_cmpxchg_emulated() returns X86EMUL_CMPXCHG_FAILED.

However, in x86_emulate_insn(), this failure appears to be silently swallowed:

arch/x86/kvm/emulate.c:x86_emulate_insn() {
    ...
    if (rc == X86EMUL_CONTINUE)
        writeback_registers(ctxt);

    return (rc == X86EMUL_UNHANDLEABLE) ? EMULATION_FAILED : EMULATION_OK;
}

Because rc is not X86EMUL_CONTINUE, writeback_registers() is skipped and the
guest registers are not updated. Because EMULATION_OK is returned, KVM retires
the instruction as successful, advancing RIP and leaving the guest with a
stale ZF=1 flag. Doesn't this cause the guest to mistakenly believe it
acquired a lock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826070004.8100-1-sarunkod@amd.com?part=2