| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <cv7xxlu3locjc66xku3hdubkictmpnoy7h2swcfxcn35po5had@nzyti7yhnbnn>
Date: Fri, 26 Dec 2025 06:52:53 +0800
From: Yao Yuan <yaoyuan0329os@...il.com>
To: Paolo Bonzini <pbonzini@...hat.com>
Cc: linux-kernel@...r.kernel.org, kvm@...r.kernel.org, seanjc@...gle.com,
x86@...nel.org, stable@...r.kernel.org
Subject: Re: [PATCH 2/5] x86, fpu: separate fpstate->xfd and guest XFD
On Wed, Dec 24, 2025 at 01:12:46AM +0100, Paolo Bonzini wrote:
> Until now, fpstate->xfd has acted as both the guest value and the value
> that the host used when executing XSAVES and XRSTORS. This is wrong: the
> data in the guest's FPU might not be initialized even if a bit is
> set in XFD and, when that happens, XRSTORing the guest FPU will fail
> with a #NM exception *on the host*.
>
> Instead, store the value of XFD together with XFD_ERR in struct
> fpu_guest; it will still be synchronized in fpu_load_guest_fpstate(), but
> the XRSTOR(S) operation will be able to load any valid state of the FPU
> independent of the XFD value.
Hi Paolo,
LGTM.
Reviewed-by: Yuan Yao <yaoyuan0329os@...il.com>
>
> Cc: stable@...r.kernel.org
> Fixes: 820a6ee944e7 ("kvm: x86: Add emulation for IA32_XFD", 2022-01-14)
> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
> ---
> arch/x86/include/asm/fpu/api.h | 6 ++----
> arch/x86/include/asm/fpu/types.h | 7 +++++++
> arch/x86/kernel/fpu/core.c | 19 ++++---------------
> arch/x86/kernel/fpu/xstate.h | 18 ++++++++++--------
> arch/x86/kvm/x86.c | 6 +++---
> 5 files changed, 26 insertions(+), 30 deletions(-)
>
> diff --git a/arch/x86/include/asm/fpu/api.h b/arch/x86/include/asm/fpu/api.h
> index 0820b2621416..ee9ba06b7dbe 100644
> --- a/arch/x86/include/asm/fpu/api.h
> +++ b/arch/x86/include/asm/fpu/api.h
> @@ -152,11 +152,9 @@ extern int fpu_swap_kvm_fpstate(struct fpu_guest *gfpu, bool enter_guest);
> extern int fpu_enable_guest_xfd_features(struct fpu_guest *guest_fpu, u64 xfeatures);
>
> #ifdef CONFIG_X86_64
> -extern void fpu_update_guest_xfd(struct fpu_guest *guest_fpu, u64 xfd);
> -extern void fpu_sync_guest_vmexit_xfd_state(void);
> +extern void fpu_sync_guest_vmexit_xfd_state(struct fpu_guest *gfpu);
> #else
> -static inline void fpu_update_guest_xfd(struct fpu_guest *guest_fpu, u64 xfd) { }
> -static inline void fpu_sync_guest_vmexit_xfd_state(void) { }
> +static inline void fpu_sync_guest_vmexit_xfd_state(struct fpu_guest *gfpu) { }
> #endif
>
> extern void fpu_copy_guest_fpstate_to_uabi(struct fpu_guest *gfpu, void *buf,
> diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h
> index 93e99d2583d6..7abe231e2ffe 100644
> --- a/arch/x86/include/asm/fpu/types.h
> +++ b/arch/x86/include/asm/fpu/types.h
> @@ -545,6 +545,13 @@ struct fpu_guest {
> */
> u64 xfeatures;
>
> + /*
> + * @xfd: Save the guest value. Note that this is
> + * *not* fpstate->xfd, which is the value
> + * the host uses when doing XSAVE/XRSTOR.
> + */
> + u64 xfd;
> +
> /*
> * @xfd_err: Save the guest value.
> */
> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> index a480fa8c65d5..ff17c96d290a 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -317,16 +317,6 @@ int fpu_enable_guest_xfd_features(struct fpu_guest *guest_fpu, u64 xfeatures)
> EXPORT_SYMBOL_FOR_KVM(fpu_enable_guest_xfd_features);
>
> #ifdef CONFIG_X86_64
> -void fpu_update_guest_xfd(struct fpu_guest *guest_fpu, u64 xfd)
> -{
> - fpregs_lock();
> - guest_fpu->fpstate->xfd = xfd;
> - if (guest_fpu->fpstate->in_use)
> - xfd_update_state(guest_fpu->fpstate);
> - fpregs_unlock();
> -}
> -EXPORT_SYMBOL_FOR_KVM(fpu_update_guest_xfd);
> -
> /**
> * fpu_sync_guest_vmexit_xfd_state - Synchronize XFD MSR and software state
> *
> @@ -339,14 +329,12 @@ EXPORT_SYMBOL_FOR_KVM(fpu_update_guest_xfd);
> * Note: It can be invoked unconditionally even when write emulation is
> * enabled for the price of a then pointless MSR read.
> */
> -void fpu_sync_guest_vmexit_xfd_state(void)
> +void fpu_sync_guest_vmexit_xfd_state(struct fpu_guest *gfpu)
> {
> - struct fpstate *fpstate = x86_task_fpu(current)->fpstate;
> -
> lockdep_assert_irqs_disabled();
> if (fpu_state_size_dynamic()) {
> - rdmsrq(MSR_IA32_XFD, fpstate->xfd);
> - __this_cpu_write(xfd_state, fpstate->xfd);
> + rdmsrq(MSR_IA32_XFD, gfpu->xfd);
> + __this_cpu_write(xfd_state, gfpu->xfd);
> }
> }
> EXPORT_SYMBOL_FOR_KVM(fpu_sync_guest_vmexit_xfd_state);
> @@ -890,6 +878,7 @@ void fpu_load_guest_fpstate(struct fpu_guest *gfpu)
> fpregs_restore_userregs();
>
> fpregs_assert_state_consistent();
> + xfd_set_state(gfpu->xfd);
> if (gfpu->xfd_err)
> wrmsrq(MSR_IA32_XFD_ERR, gfpu->xfd_err);
> }
> diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h
> index 52ce19289989..c0ce05bee637 100644
> --- a/arch/x86/kernel/fpu/xstate.h
> +++ b/arch/x86/kernel/fpu/xstate.h
> @@ -180,26 +180,28 @@ static inline void xfd_validate_state(struct fpstate *fpstate, u64 mask, bool rs
> #endif
>
> #ifdef CONFIG_X86_64
> -static inline void xfd_set_state(u64 xfd)
> +static inline void __xfd_set_state(u64 xfd)
> {
> wrmsrq(MSR_IA32_XFD, xfd);
> __this_cpu_write(xfd_state, xfd);
> }
>
> +static inline void xfd_set_state(u64 xfd)
> +{
> + if (__this_cpu_read(xfd_state) != xfd)
> + __xfd_set_state(xfd);
> +}
> +
> static inline void xfd_update_state(struct fpstate *fpstate)
> {
> - if (fpu_state_size_dynamic()) {
> - u64 xfd = fpstate->xfd;
> -
> - if (__this_cpu_read(xfd_state) != xfd)
> - xfd_set_state(xfd);
> - }
> + if (fpu_state_size_dynamic())
> + xfd_set_state(fpstate->xfd);
> }
>
> extern int __xfd_enable_feature(u64 which, struct fpu_guest *guest_fpu);
> #else
> static inline void xfd_set_state(u64 xfd) { }
> -
> +static inline void __xfd_set_state(u64 xfd) { }
> static inline void xfd_update_state(struct fpstate *fpstate) { }
>
> static inline int __xfd_enable_feature(u64 which, struct fpu_guest *guest_fpu) {
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 01d95192dfc5..56fd082859bc 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4261,7 +4261,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> if (data & ~kvm_guest_supported_xfd(vcpu))
> return 1;
>
> - fpu_update_guest_xfd(&vcpu->arch.guest_fpu, data);
> + vcpu->arch.guest_fpu.xfd = data;
> break;
> case MSR_IA32_XFD_ERR:
> if (!msr_info->host_initiated &&
> @@ -4617,7 +4617,7 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> !guest_cpu_cap_has(vcpu, X86_FEATURE_XFD))
> return 1;
>
> - msr_info->data = vcpu->arch.guest_fpu.fpstate->xfd;
> + msr_info->data = vcpu->arch.guest_fpu.xfd;
> break;
> case MSR_IA32_XFD_ERR:
> if (!msr_info->host_initiated &&
> @@ -11405,7 +11405,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> * in #NM irqoff handler).
> */
> if (vcpu->arch.xfd_no_write_intercept)
> - fpu_sync_guest_vmexit_xfd_state();
> + fpu_sync_guest_vmexit_xfd_state(&vcpu->arch.guest_fpu);
>
> kvm_x86_call(handle_exit_irqoff)(vcpu);
>
> --
> 2.52.0
>
>
Powered by blists - more mailing lists