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

[sashiko-bot@kernel.org]

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

New issues:
- [High] KVM fails to zero out APX registers (R16-R31) when restoring an XSAVE buffer where the APX feature bit is cleared, leaking stale state across snapshot restorations.

Pre-existing issues:
- [Critical] Enabling APX support inherently inflates the default FPU uABI size beyond 4096 bytes, unconditionally breaking the legacy KVM_GET_XSAVE ioctl on APX-capable hosts.
--

commit e523aa83189e420a1aea25a00998b66b7d7b10d8
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 for the XSAVE ABI. It adds two copy functions that
run during state export and import to ensure APX state is correctly handled
since the VCPU cache acts as the single source of truth.

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

[Severity: Critical]
This is a pre-existing issue, but does enabling APX support inherently
inflate the default FPU uABI size beyond 4096 bytes, unconditionally
breaking the legacy KVM_GET_XSAVE ioctl on APX-capable hosts?

In arch/x86/kvm/x86.c:kvm_arch_vcpu_ioctl(), the legacy KVM_GET_XSAVE
ioctl unconditionally rejects the call if the size is too large:

	case KVM_GET_XSAVE: {
		r = -EINVAL;
		if (vcpu->arch.guest_fpu.uabi_size > sizeof(struct kvm_xsave))
			break;

Because APX statically inflates the host's default_size beyond 4096 bytes
(the size of struct kvm_xsave), and the core FPU subsystem initializes
vcpu->arch.guest_fpu.uabi_size to this host default size, the legacy ioctl
will instantly fail with -EINVAL. Can this regression prevent VM boot,
migration, or snapshotting for legacy userspace (e.g., older versions of
QEMU)?

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 79468ddfe4736..0df74422759f8 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
[ ... ]
> @@ -3141,6 +3141,49 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
>  	return 0;
>  }
>  
> +#ifdef CONFIG_KVM_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]
When restoring an XSAVE buffer where the XFEATURE_MASK_APX bit is cleared
in the header, does this code fail to zero out the APX registers (R16-R31)
in the software vCPU cache?

Since the vCPU cache is the single source of truth, returning immediately
leaves stale non-zero values from a previous vCPU execution. Will these
stale values be incorrectly loaded into hardware during VM-entry, violating
the x86 architecture requirement that missing XSAVE features are restored to
their INIT (zero) state?

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

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