Re: [PATCH v5 03/20] KVM: x86: Support APX state for XSAVE ABI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] KVM_SET_XSAVE fails to reset APX registers to their architectural INIT state (zero) when the APX bit is missing from the provided XSAVE header.
- [High] Adding APX as a static FPU feature inflates KVM's base uABI size beyond 4096 bytes on AMX-capable hosts, unconditionally breaking legacy KVM_GET_XSAVE and causing out-of-bounds userspace memory reads in KVM_SET_XSAVE.
- [High] kvm_copy_uabi_to_vcpu_regs() modifies vcpu->arch.regs before the rest of the FPU state is validated, leading to a partial VCPU state update if the ioctl fails.
--

--- Patch [3]: [PATCH v5 03/20] KVM: x86: Support APX state for XSAVE ABI ---
commit abf6cc0d5ca657b7c271fda294578cfed25f0046
Author: Chang S. Bae <chang.seok.bae@intel.com>

KVM: x86: Support APX state for XSAVE ABI

Introduce a facility to copy APX state between the VCPU cache and the
userspace buffer as VCPU cache is the single source of truth.

[ ... ]

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 0626e835e9eb7..5578df10179ae 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> +#ifdef CONFIG_KVM_APX
> +static void kvm_copy_vcpu_regs_to_uabi(struct kvm_vcpu *vcpu, void *buf, u64 supported_xcr0)
> +{

[ ... ]

> +}
> +
> +static int kvm_copy_uabi_to_vcpu_regs(struct kvm_vcpu *vcpu, void *buf, u64 supported_xcr0)
> +{
> +	union fpregs_state *xstate = (union fpregs_state *)buf;
> +
> +	if (!(xstate->xsave.header.xfeatures & XFEATURE_MASK_APX))
> +		return 0;

[Severity: High]
Does this early return fail to reset the APX registers to their architectural
INIT state?

If a VMM restores a vCPU's state using KVM_SET_XSAVE and the
XFEATURE_MASK_APX bit is clear, the APX component should be initialized to
zero. Because this returns early without modifying vcpu->arch.regs, could
this leave stale data in the VCPU's APX registers and leak state across
resets or migrations?

> +
> +	if (!(supported_xcr0 & XFEATURE_MASK_APX))
> +		return -EINVAL;

[ ... ]

> @@ -3161,6 +3211,7 @@ static int kvm_vcpu_ioctl_x86_set_xsave(struct kvm_vcpu *vcpu,
>  					struct kvm_xsave *guest_xsave)
>  {
>  	union fpregs_state *xstate = (union fpregs_state *)guest_xsave->region;

[Severity: High]
Will adding APX as a static FPU feature inflate KVM's base uABI size beyond
4096 bytes on AMX-capable hosts, unconditionally breaking legacy KVM_GET_XSAVE?

Because APX (a static feature) is architecturally located after AMX (a
dynamic feature) in the uncompacted XSAVE format, its presence expands
fpu_user_cfg.default_size to > 10KB. KVM expands uabi_size to this value,
which can cause legacy VMMs passing a standard 4096-byte struct kvm_xsave
buffer to trigger an out-of-bounds read in KVM_SET_XSAVE:

arch/x86/kvm/x86.c:kvm_arch_vcpu_ioctl() {
    case KVM_SET_XSAVE: {
        int size = vcpu->arch.guest_fpu.uabi_size;
        u.xsave = memdup_user(argp, size);
        ...
}

It also appears KVM_GET_XSAVE unconditionally fails because it passes a size
of 4096 to kvm_vcpu_ioctl_x86_get_xsave2(), which rejects it if
size < uabi_size:

arch/x86/kvm/x86.c:kvm_vcpu_ioctl_x86_get_xsave() {
    return kvm_vcpu_ioctl_x86_get_xsave2(vcpu, (void *)guest_xsave->region,
                                         sizeof(guest_xsave->region));
}

Is there a way to handle the legacy 4096-byte buffer size gracefully here?

> +	int err;
>  
>  	if (fpstate_is_confidential(&vcpu->arch.guest_fpu))
>  		return vcpu->kvm->arch.has_protected_state ? -EINVAL : 0;

[ ... ]

> @@ -3172,6 +3223,16 @@ static int kvm_vcpu_ioctl_x86_set_xsave(struct kvm_vcpu *vcpu,
>  	 */
>  	xstate->xsave.header.xfeatures &= ~vcpu->arch.guest_fpu.fpstate->xfd;
>  
> +	/*
> +	 * This and the following copy functions copy APX state into each
> +	 * storage. The VCPU cache is the single source of truth but avoiding
> +	 * this redundant copy tends to introduce special-case handling. Since
> +	 * this lies in a slow path, keep the implementation simple.
> +	 */
> +	err = kvm_copy_uabi_to_vcpu_regs(vcpu, guest_xsave->region, kvm_caps.supported_xcr0);
> +	if (err)
> +		return err;

[Severity: High]
Does modifying the APX state before validating the rest of the FPU state
lead to a corrupted vCPU state if the ioctl fails?

Looking at kvm_copy_uabi_to_vcpu_regs(), it copies the APX state directly
into vcpu->arch.regs. If fpu_copy_uabi_to_guest_fpstate() subsequently
fails validation (for example, due to malformed MXCSR reserved bits), the
ioctl aborts and returns an error. 

Since the APX registers were already permanently modified, could this violate
transactional ioctl semantics and leave the vCPU in an inconsistent state?

> +
>  	return fpu_copy_uabi_to_guest_fpstate(&vcpu->arch.guest_fpu,
>  					      guest_xsave->region,
>  					      kvm_caps.supported_xcr0,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720171949.498680-1-chang.seok.bae@intel.com?part=3