From: "Mika Penttilä" <mika.penttila@nextfour.com>
To: "Chang S. Bae" <chang.seok.bae@intel.com>, linux-kernel@vger.kernel.org
Cc: x86@kernel.org, tglx@linutronix.de, dave.hansen@linux.intel.com,
arjan@linux.intel.com, ravi.v.shankar@intel.com
Subject: Re: [PATCH 15/23] x86/fpu: Add sanity checks for XFD
Date: Mon, 25 Oct 2021 11:33:05 +0300 [thread overview]
Message-ID: <20d31ed9-be3d-dca6-ceef-ced35f80d131@nextfour.com> (raw)
In-Reply-To: <20211021225527.10184-16-chang.seok.bae@intel.com>
On 22.10.2021 1.55, Chang S. Bae wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> Add debug functionality to ensure that the XFD MSR is up to date for XSAVE*
> and XRSTOR* operations.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
> ---
> Changes from the tglx tree:
> * Rename the xstate ops validation helper. (Dave Hansen)
> * Update code comments. (Dave Hansen)
> ---
> arch/x86/kernel/fpu/core.c | 9 +++---
> arch/x86/kernel/fpu/signal.c | 6 ++--
> arch/x86/kernel/fpu/xstate.c | 55 ++++++++++++++++++++++++++++++++++++
> arch/x86/kernel/fpu/xstate.h | 34 +++++++++++++++++++---
> 4 files changed, 92 insertions(+), 12 deletions(-)
>
> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> index e92c9d79441c..f4b02ed47034 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -166,7 +166,7 @@ void restore_fpregs_from_fpstate(struct fpstate *fpstate, u64 mask)
> */
> mask = fpu_kernel_cfg.max_features & mask;
>
> - os_xrstor(&fpstate->regs.xsave, mask);
> + os_xrstor(fpstate, mask);
> } else {
> if (use_fxsr())
> fxrstor(&fpstate->regs.fxsave);
> @@ -537,7 +537,7 @@ void fpu__drop(struct fpu *fpu)
> static inline void restore_fpregs_from_init_fpstate(u64 features_mask)
> {
> if (use_xsave())
> - os_xrstor(&init_fpstate.regs.xsave, features_mask);
> + os_xrstor(&init_fpstate, features_mask);
> else if (use_fxsr())
> fxrstor(&init_fpstate.regs.fxsave);
> else
> @@ -594,9 +594,8 @@ void fpu__clear_user_states(struct fpu *fpu)
> * corresponding registers.
> */
> if (xfeatures_mask_supervisor() &&
> - !fpregs_state_valid(fpu, smp_processor_id())) {
> - os_xrstor(&fpu->fpstate->regs.xsave, xfeatures_mask_supervisor());
> - }
> + !fpregs_state_valid(fpu, smp_processor_id()))
> + os_xrstor_supervisor(fpu->fpstate);
>
> /* Reset user states in registers. */
> restore_fpregs_from_init_fpstate(XFEATURE_MASK_USER_RESTORE);
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index d128eb581ffc..a937980fd02b 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -261,7 +261,7 @@ static int __restore_fpregs_from_user(void __user *buf, u64 ufeatures,
> ret = fxrstor_from_user_sigframe(buf);
>
> if (!ret && unlikely(init_bv))
> - os_xrstor(&init_fpstate.regs.xsave, init_bv);
> + os_xrstor(&init_fpstate, init_bv);
> return ret;
> } else if (use_fxsr()) {
> return fxrstor_from_user_sigframe(buf);
> @@ -322,7 +322,7 @@ static bool restore_fpregs_from_user(void __user *buf, u64 xrestore,
> * been restored from a user buffer directly.
> */
> if (test_thread_flag(TIF_NEED_FPU_LOAD) && xfeatures_mask_supervisor())
> - os_xrstor(&fpu->fpstate->regs.xsave, xfeatures_mask_supervisor());
> + os_xrstor_supervisor(fpu->fpstate);
>
> fpregs_mark_activate();
> fpregs_unlock();
> @@ -432,7 +432,7 @@ static bool __fpu_restore_sig(void __user *buf, void __user *buf_fx,
> u64 mask = user_xfeatures | xfeatures_mask_supervisor();
>
> fpregs->xsave.header.xfeatures &= mask;
> - success = !os_xrstor_safe(&fpregs->xsave,
> + success = !os_xrstor_safe(fpu->fpstate,
> fpu_kernel_cfg.max_features);
> } else {
> success = !fxrstor_safe(&fpregs->fxsave);
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index c178aa91abb3..c2cbe14aaa00 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -1301,6 +1301,61 @@ EXPORT_SYMBOL_GPL(fpstate_clear_xstate_component);
> #endif
>
> #ifdef CONFIG_X86_64
> +
> +#ifdef CONFIG_X86_DEBUG_FPU
> +/*
> + * Ensure that a subsequent XSAVE* or XRSTOR* instruction with RFBM=@mask
> + * can safely operate on the @fpstate buffer.
> + */
> +static bool xstate_op_valid(struct fpstate *fpstate, u64 mask, bool rstor)
> +{
> + u64 xfd = __this_cpu_read(xfd_state);
> +
> + if (fpstate->xfd == xfd)
> + return true;
> +
> + /* For current's fpstate the XFD state must be correct. */
> + if (fpstate->xfd == current->thread.fpu.fpstate->xfd)
> + return false;
> +
Should this return true or is the comment confusing?
> + /*
> + * XRSTOR(S) from init_fpstate are always correct as it will just
> + * bring all components into init state and not read from the
> + * buffer. XSAVE(S) raises #PF after init.
> + */
> + if (fpstate == &init_fpstate)
> + return rstor;
> +
> + /*
> + * XSAVE(S): clone(), fpu_swap_kvm_fpu()
> + * XRSTORS(S): fpu_swap_kvm_fpu()
> + */
> +
> + /*
> + * No XSAVE/XRSTOR instructions (except XSAVE itself) touch
> + * the buffer area for XFD-disabled state components.
> + */
> + mask &= ~xfd;
> +
> + /*
> + * Remove features which are valid in fpstate. They
> + * have space allocated in fpstate.
> + */
> + mask &= ~fpstate->xfeatures;
> +
> + /*
> + * Any remaining state components in 'mask' might be written
> + * by XSAVE/XRSTOR. Fail validation it found.
> + */
> + return !mask;
> +}
> +
> +void xfd_validate_state(struct fpstate *fpstate, u64 mask, bool rstor)
> +{
> + WARN_ON_ONCE(!xstate_op_valid(fpstate, mask, rstor));
> +}
> +#endif /* CONFIG_X86_DEBUG_FPU */
> +
> static int validate_sigaltstack(unsigned int usize)
> {
> struct task_struct *thread, *leader = current->group_leader;
> diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h
> index 32a4dee4de3b..29024244965b 100644
> --- a/arch/x86/kernel/fpu/xstate.h
> +++ b/arch/x86/kernel/fpu/xstate.h
> @@ -130,6 +130,12 @@ static inline u64 xfeatures_mask_independent(void)
> : "D" (st), "m" (*st), "a" (lmask), "d" (hmask) \
> : "memory")
>
> +#if defined(CONFIG_X86_64) && defined(CONFIG_X86_DEBUG_FPU)
> +extern void xfd_validate_state(struct fpstate *fpstate, u64 mask, bool rstor);
> +#else
> +static inline void xfd_validate_state(struct fpstate *fpstate, u64 mask, bool rstor) { }
> +#endif
> +
> /*
> * Save processor xstate to xsave area.
> *
> @@ -144,6 +150,7 @@ static inline void os_xsave(struct fpstate *fpstate)
> int err;
>
> WARN_ON_FPU(!alternatives_patched);
> + xfd_validate_state(fpstate, mask, false);
>
> XSTATE_XSAVE(&fpstate->regs.xsave, lmask, hmask, err);
>
> @@ -156,12 +163,23 @@ static inline void os_xsave(struct fpstate *fpstate)
> *
> * Uses XRSTORS when XSAVES is used, XRSTOR otherwise.
> */
> -static inline void os_xrstor(struct xregs_state *xstate, u64 mask)
> +static inline void os_xrstor(struct fpstate *fpstate, u64 mask)
> +{
> + u32 lmask = mask;
> + u32 hmask = mask >> 32;
> +
> + xfd_validate_state(fpstate, mask, true);
> + XSTATE_XRESTORE(&fpstate->regs.xsave, lmask, hmask);
> +}
> +
> +/* Restore of supervisor state. Does not require XFD */
> +static inline void os_xrstor_supervisor(struct fpstate *fpstate)
> {
> + u64 mask = xfeatures_mask_supervisor();
> u32 lmask = mask;
> u32 hmask = mask >> 32;
>
> - XSTATE_XRESTORE(xstate, lmask, hmask);
> + XSTATE_XRESTORE(&fpstate->regs.xsave, lmask, hmask);
> }
>
> /*
> @@ -184,11 +202,14 @@ static inline int xsave_to_user_sigframe(struct xregs_state __user *buf)
> * internally, e.g. PKRU. That's user space ABI and also required
> * to allow the signal handler to modify PKRU.
> */
> - u64 mask = current->thread.fpu.fpstate->user_xfeatures;
> + struct fpstate *fpstate = current->thread.fpu.fpstate;
> + u64 mask = fpstate->user_xfeatures;
> u32 lmask = mask;
> u32 hmask = mask >> 32;
> int err;
>
> + xfd_validate_state(fpstate, mask, false);
> +
> stac();
> XSTATE_OP(XSAVE, buf, lmask, hmask, err);
> clac();
> @@ -206,6 +227,8 @@ static inline int xrstor_from_user_sigframe(struct xregs_state __user *buf, u64
> u32 hmask = mask >> 32;
> int err;
>
> + xfd_validate_state(current->thread.fpu.fpstate, mask, true);
> +
> stac();
> XSTATE_OP(XRSTOR, xstate, lmask, hmask, err);
> clac();
> @@ -217,12 +240,15 @@ static inline int xrstor_from_user_sigframe(struct xregs_state __user *buf, u64
> * Restore xstate from kernel space xsave area, return an error code instead of
> * an exception.
> */
> -static inline int os_xrstor_safe(struct xregs_state *xstate, u64 mask)
> +static inline int os_xrstor_safe(struct fpstate *fpstate, u64 mask)
> {
> + struct xregs_state *xstate = &fpstate->regs.xsave;
> u32 lmask = mask;
> u32 hmask = mask >> 32;
> int err;
>
> + /* Must enforce XFD update here */
> +
> if (cpu_feature_enabled(X86_FEATURE_XSAVES))
> XSTATE_OP(XRSTORS, xstate, lmask, hmask, err);
> else
next prev parent reply other threads:[~2021-10-25 8:33 UTC|newest]
Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-21 22:55 [PATCH 00/23] x86: Support Intel Advanced Matrix Extensions (part 4) Chang S. Bae
2021-10-21 22:55 ` [PATCH 01/23] signal: Add an optional check for altstack size Chang S. Bae
2021-10-22 0:06 ` Bae, Chang Seok
2021-10-22 15:20 ` Eric W. Biederman
2021-10-22 20:58 ` Bae, Chang Seok
2021-10-22 22:51 ` Dave Hansen
2021-10-26 16:16 ` [tip: x86/fpu] " tip-bot2 for Thomas Gleixner
2021-10-21 22:55 ` [PATCH 02/23] x86/signal: Implement sigaltstack size validation Chang S. Bae
2021-10-26 16:16 ` [tip: x86/fpu] " tip-bot2 for Thomas Gleixner
2021-10-21 22:55 ` [PATCH 03/23] x86/fpu/xstate: Provide xstate_calculate_size() Chang S. Bae
2021-10-26 16:16 ` [tip: x86/fpu] " tip-bot2 for Chang S. Bae
2021-10-21 22:55 ` [PATCH 04/23] x86/fpu: Add members to struct fpu to cache permission information Chang S. Bae
2021-10-26 16:16 ` [tip: x86/fpu] " tip-bot2 for Thomas Gleixner
2021-10-21 22:55 ` [PATCH 05/23] x86/fpu: Add fpu_state_config::legacy_features Chang S. Bae
2021-10-26 16:16 ` [tip: x86/fpu] " tip-bot2 for Thomas Gleixner
2021-10-21 22:55 ` [PATCH 06/23] x86/arch_prctl: Add controls for dynamic XSTATE components Chang S. Bae
2021-10-24 21:17 ` Borislav Petkov
2021-10-26 9:11 ` [PATCH] Documentation/x86: Add documentation for using dynamic XSTATE features Chang S. Bae
2021-10-26 16:16 ` [tip: x86/fpu] " tip-bot2 for Chang S. Bae
2021-10-28 13:10 ` tip-bot2 for Chang S. Bae
2021-10-26 16:16 ` [tip: x86/fpu] x86/arch_prctl: Add controls for dynamic XSTATE components tip-bot2 for Chang S. Bae
2021-10-21 22:55 ` [PATCH 07/23] x86/fpu: Add basic helpers for dynamically enabled features Chang S. Bae
2021-10-26 16:16 ` [tip: x86/fpu] " tip-bot2 for Thomas Gleixner
2021-10-21 22:55 ` [PATCH 08/23] x86/signal: Use fpu::__state_user_size for sigalt stack validation Chang S. Bae
2021-10-26 16:16 ` [tip: x86/fpu] " tip-bot2 for Thomas Gleixner
2021-10-21 22:55 ` [PATCH 09/23] x86/fpu/signal: Prepare for variable sigframe length Chang S. Bae
2021-10-26 16:16 ` [tip: x86/fpu] " tip-bot2 for Chang S. Bae
2021-10-21 22:55 ` [PATCH 10/23] x86/fpu: Prepare fpu_clone() for dynamically enabled features Chang S. Bae
2021-10-26 16:16 ` [tip: x86/fpu] " tip-bot2 for Thomas Gleixner
2021-10-21 22:55 ` [PATCH 11/23] x86/fpu: Reset permission and fpstate on exec() Chang S. Bae
2021-10-26 16:16 ` [tip: x86/fpu] " tip-bot2 for Chang S. Bae
2021-10-21 22:55 ` [PATCH 12/23] x86/cpufeatures: Add eXtended Feature Disabling (XFD) feature bit Chang S. Bae
2021-10-26 16:16 ` [tip: x86/fpu] " tip-bot2 for Chang S. Bae
2021-10-21 22:55 ` [PATCH 13/23] x86/msr-index: Add MSRs for XFD Chang S. Bae
2021-10-26 16:16 ` [tip: x86/fpu] " tip-bot2 for Chang S. Bae
2021-10-21 22:55 ` [PATCH 14/23] x86/fpu: Add XFD state to fpstate Chang S. Bae
2021-10-26 16:16 ` [tip: x86/fpu] " tip-bot2 for Chang S. Bae
2021-10-21 22:55 ` [PATCH 15/23] x86/fpu: Add sanity checks for XFD Chang S. Bae
2021-10-25 8:11 ` Borislav Petkov
2021-10-25 20:15 ` Thomas Gleixner
2021-10-25 8:33 ` Mika Penttilä [this message]
2021-10-25 18:13 ` Thomas Gleixner
2021-10-25 19:57 ` Dave Hansen
2021-10-26 16:16 ` [tip: x86/fpu] " tip-bot2 for Thomas Gleixner
2021-10-21 22:55 ` [PATCH 16/23] x86/fpu: Update XFD state where required Chang S. Bae
2021-10-26 16:16 ` [tip: x86/fpu] " tip-bot2 for Chang S. Bae
2021-10-21 22:55 ` [PATCH 17/23] x86/fpu/xstate: Add XFD #NM handler Chang S. Bae
2021-10-26 16:16 ` [tip: x86/fpu] " tip-bot2 for Chang S. Bae
2021-10-21 22:55 ` [PATCH 18/23] x86/fpu/xstate: Add fpstate_realloc()/free() Chang S. Bae
2021-10-26 16:16 ` [tip: x86/fpu] " tip-bot2 for Chang S. Bae
2021-10-21 22:55 ` [PATCH 19/23] x86/fpu/xstate: Prepare XSAVE feature table for gaps in state component numbers Chang S. Bae
2021-10-26 16:16 ` [tip: x86/fpu] " tip-bot2 for Chang S. Bae
2021-10-21 22:55 ` [PATCH 20/23] x86/fpu/amx: Define AMX state components and have it used for boot-time checks Chang S. Bae
2021-10-26 16:16 ` [tip: x86/fpu] " tip-bot2 for Chang S. Bae
2021-10-21 22:55 ` [PATCH 21/23] x86/fpu: Calculate the default sizes independently Chang S. Bae
2021-10-26 16:16 ` [tip: x86/fpu] " tip-bot2 for Chang S. Bae
2021-10-21 22:55 ` [PATCH 22/23] x86/fpu: Add XFD handling for dynamic states Chang S. Bae
2021-10-26 16:16 ` [tip: x86/fpu] " tip-bot2 for Chang S. Bae
2021-10-21 22:55 ` [PATCH 23/23] x86/fpu/amx: Enable the AMX feature in 64-bit mode Chang S. Bae
2021-10-26 16:16 ` [tip: x86/fpu] " tip-bot2 for Chang S. Bae
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20d31ed9-be3d-dca6-ceef-ced35f80d131@nextfour.com \
--to=mika.penttila@nextfour.com \
--cc=arjan@linux.intel.com \
--cc=chang.seok.bae@intel.com \
--cc=dave.hansen@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=ravi.v.shankar@intel.com \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox