Re: [PATCH v6 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:

New issues:
- [High] State Leak / Incomplete Reset for APX EGPRs
- [High] APX Support is Dead Code / Broken via XCR0 Masking

Pre-existing issues:
- [High] ABI Breakage of KVM_GET_XSAVE on CPUs with APX
--

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

This commit introduces a facility to copy APX state between the VCPU cache and
the userspace buffer. It handles the APX EGPRs specifically, copying them
to and from vcpu->arch.regs when the APX feature bit is set.

> 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
[ ... ]
> @@ -3124,6 +3124,49 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
>  	return 0;
>  }
>  
> +#ifdef CONFIG_KVM_APX
> +static void kvm_copy_vcpu_regs_to_uabi(struct kvm_vcpu *vcpu, void *buf, u64 supported_xcr0)
> +{
> +	union fpregs_state *xstate = (union fpregs_state *)buf;
> +
> +	BUILD_BUG_ON(NR_VCPU_GENERAL_PURPOSE_REGS <= VCPU_REGS_R31);
> +
> +	if (!(supported_xcr0 & XFEATURE_MASK_APX))
> +		return;
> +
> +	memcpy(buf + xstate_offset(XFEATURE_APX),
> +	       &vcpu->arch.regs[VCPU_REGS_R16],
> +	       xstate_size(XFEATURE_APX));
> +
> +	xstate->xsave.header.xfeatures |= XFEATURE_MASK_APX;
> +}
> +
> +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 inadvertently leak the values of APX extended GPRs
across VM resets or migrations?

If userspace calls KVM_SET_XSAVE with XFEATURE_MASK_APX cleared in the
XSAVE header, the XSAVE INIT state semantics dictate that the corresponding
state should be reset. By returning 0 early here, KVM fails to zero out
vcpu->arch.regs[VCPU_REGS_R16] through vcpu->arch.regs[VCPU_REGS_R31]. KVM
will then manually load these stale register values into the CPU during
VM-entry.

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

[Severity: High]
Does this code inadvertently make APX support dead code by failing to add
XFEATURE_MASK_APX to KVM_SUPPORTED_XCR0?

Because XFEATURE_MASK_APX is omitted from KVM_SUPPORTED_XCR0, it is stripped
from KVM's capabilities. This means supported_xcr0 will never contain the APX
bit, causing KVM_SET_XSAVE to unconditionally fail with -EINVAL here, and
preventing APX state from being exported during KVM_GET_XSAVE2.

> +
> +	BUILD_BUG_ON(NR_VCPU_GENERAL_PURPOSE_REGS <= VCPU_REGS_R31);
> +
> +	memcpy(&vcpu->arch.regs[VCPU_REGS_R16],
> +	       buf + xstate_offset(XFEATURE_APX),
> +	       xstate_size(XFEATURE_APX));
> +
> +	return 0;
> +}
[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but there is an ABI breakage with
KVM_GET_XSAVE on CPUs with APX that affects this feature area.

Because APX is not defined as an XFD dynamic feature, it is unconditionally
included in fpu_user_cfg.default_features. On CPUs supporting both AMX and APX,
the uncompacted offset of APX pushes fpu_user_cfg.default_size over 4096 bytes.

During VCPU creation in fpu_alloc_guest_fpstate():

	gfpu->uabi_size		= sizeof(struct kvm_xsave);
	if (WARN_ON_ONCE(fpu_user_cfg.default_size > gfpu->uabi_size))
		gfpu->uabi_size = fpu_user_cfg.default_size;

This inflates gfpu->uabi_size to match default_size. Later, calls to the
legacy KVM_GET_XSAVE ioctl unconditionally fail with -EINVAL because they
enforce uabi_size <= sizeof(struct kvm_xsave) (4096 bytes), breaking backwards
compatibility for older VMMs.

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