| 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
| ||
|
Date: Mon, 21 Jun 2021 16:42:53 -0400
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Thomas Gleixner <tglx@...utronix.de>,
LKML <linux-kernel@...r.kernel.org>
Cc: Andy Lutomirski <luto@...nel.org>,
Dave Hansen <dave.hansen@...ux.intel.com>,
Fenghua Yu <fenghua.yu@...el.com>,
Tony Luck <tony.luck@...el.com>,
Yu-cheng Yu <yu-cheng.yu@...el.com>,
Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
Borislav Petkov <bp@...e.de>,
Peter Zijlstra <peterz@...radead.org>
Subject: Re: [patch V3 38/66] x86/fpu/xstate: Sanitize handling of independent
features
On 6/18/2021 10:19 AM, Thomas Gleixner wrote:
> The copy functions for the independent features are horribly named and the
> supervisor and independent part is just overengineered.
>
> The point is that the supplied mask has either to be a subset of the
> independent feature or a subset of the task->fpu.xstate managed features.
>
> Rewrite it so it checks check for invalid overlaps of these areas in the
> caller supplied feature mask. Rename it so it follows the new naming
> convention for these operations. Mop up the function documentation.
>
> This allows to use that function for other purposes as well.
>
> Suggested-by: Peter Zijlstra <peterz@...radead.org>
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> Cc: Kan Liang <kan.liang@...ux.intel.com>
> ---
I have tested the LBR Xsave feature. Everything looks good.
Tested-by: Kan Liang <kan.liang@...ux.intel.com>
Thanks,
Kan
> V3: Rename
> ---
> arch/x86/events/intel/lbr.c | 6 +-
> arch/x86/include/asm/fpu/xstate.h | 5 +-
> arch/x86/kernel/fpu/xstate.c | 93 +++++++++++++++++++-------------------
> 3 files changed, 53 insertions(+), 51 deletions(-)
>
> --- a/arch/x86/events/intel/lbr.c
> +++ b/arch/x86/events/intel/lbr.c
> @@ -491,7 +491,7 @@ static void intel_pmu_arch_lbr_xrstors(v
> {
> struct x86_perf_task_context_arch_lbr_xsave *task_ctx = ctx;
>
> - copy_kernel_to_independent_supervisor(&task_ctx->xsave, XFEATURE_MASK_LBR);
> + xrstors(&task_ctx->xsave, XFEATURE_MASK_LBR);
> }
>
> static __always_inline bool lbr_is_reset_in_cstate(void *ctx)
> @@ -576,7 +576,7 @@ static void intel_pmu_arch_lbr_xsaves(vo
> {
> struct x86_perf_task_context_arch_lbr_xsave *task_ctx = ctx;
>
> - copy_independent_supervisor_to_kernel(&task_ctx->xsave, XFEATURE_MASK_LBR);
> + xsaves(&task_ctx->xsave, XFEATURE_MASK_LBR);
> }
>
> static void __intel_pmu_lbr_save(void *ctx)
> @@ -992,7 +992,7 @@ static void intel_pmu_arch_lbr_read_xsav
> intel_pmu_store_lbr(cpuc, NULL);
> return;
> }
> - copy_independent_supervisor_to_kernel(&xsave->xsave, XFEATURE_MASK_LBR);
> + xsaves(&xsave->xsave, XFEATURE_MASK_LBR);
>
> intel_pmu_store_lbr(cpuc, xsave->lbr.entries);
> }
> --- a/arch/x86/include/asm/fpu/xstate.h
> +++ b/arch/x86/include/asm/fpu/xstate.h
> @@ -104,8 +104,9 @@ void *get_xsave_addr(struct xregs_state
> int xfeature_size(int xfeature_nr);
> int copy_uabi_from_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf);
> int copy_sigframe_from_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf);
> -void copy_independent_supervisor_to_kernel(struct xregs_state *xstate, u64 mask);
> -void copy_kernel_to_independent_supervisor(struct xregs_state *xstate, u64 mask);
> +
> +void xsaves(struct xregs_state *xsave, u64 mask);
> +void xrstors(struct xregs_state *xsave, u64 mask);
>
> enum xstate_copy_mode {
> XSTATE_COPY_FP,
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -1163,75 +1163,76 @@ int copy_sigframe_from_user_to_xstate(st
> }
>
> /**
> - * copy_independent_supervisor_to_kernel() - Save independent supervisor states to
> - * an xsave area
> - * @xstate: A pointer to an xsave area
> - * @mask: Represent the independent supervisor features saved into the xsave area
> + * xsaves - Save selected components to a kernel xstate buffer
> + * @xstate: Pointer to the buffer
> + * @mask: Feature mask to select the components to save
> *
> - * Only the independent supervisor states sets in the mask are saved into the xsave
> - * area (See the comment in XFEATURE_MASK_INDEPENDENT for the details of independent
> - * supervisor feature). Besides the independent supervisor states, the legacy
> - * region and XSAVE header are also saved into the xsave area. The supervisor
> - * features in the XFEATURE_MASK_SUPERVISOR_SUPPORTED and
> - * XFEATURE_MASK_SUPERVISOR_UNSUPPORTED are not saved.
> + * The @xstate buffer must be 64 byte aligned and correctly initialized as
> + * XSAVES does not write the full xstate header. Before first use the
> + * buffer should be zeroed otherwise a consecutive XRSTORS from that buffer
> + * can #GP.
> *
> - * The xsave area must be 64-bytes aligned.
> + * The feature mask must either be a subset of the independent features or
> + * a subset of the task->fpstate related features
> */
> -void copy_independent_supervisor_to_kernel(struct xregs_state *xstate, u64 mask)
> +void xsaves(struct xregs_state *xstate, u64 mask)
> {
> - u64 independent_mask = xfeatures_mask_independent() & mask;
> - u32 lmask, hmask;
> + u64 xchk;
> int err;
>
> - if (WARN_ON_FPU(!boot_cpu_has(X86_FEATURE_XSAVES)))
> + if (WARN_ON_FPU(!cpu_feature_enabled(X86_FEATURE_XSAVES)))
> return;
> + /*
> + * Validate that this is either a task->fpstate related component
> + * subset or an independent one.
> + */
> + if (mask & xfeatures_mask_independent())
> + xchk = ~xfeatures_mask_independent();
> + else
> + xchk = ~xfeatures_mask_all;
>
> - if (WARN_ON_FPU(!independent_mask))
> + if (WARN_ON_ONCE(!mask || mask & xchk))
> return;
>
> - lmask = independent_mask;
> - hmask = independent_mask >> 32;
> -
> - XSTATE_OP(XSAVES, xstate, lmask, hmask, err);
> -
> - /* Should never fault when copying to a kernel buffer */
> - WARN_ON_FPU(err);
> + XSTATE_OP(XSAVES, xstate, (u32)mask, (u32)(mask >> 32), err);
> + WARN_ON_ONCE(err);
> }
>
> /**
> - * copy_kernel_to_independent_supervisor() - Restore independent supervisor states from
> - * an xsave area
> - * @xstate: A pointer to an xsave area
> - * @mask: Represent the independent supervisor features restored from the xsave area
> + * xrstors - Restore selected components from a kernel xstate buffer
> + * @xstate: Pointer to the buffer
> + * @mask: Feature mask to select the components to restore
> + *
> + * The @xstate buffer must be 64 byte aligned and correctly initialized
> + * otherwise XRSTORS from that buffer can #GP.
> *
> - * Only the independent supervisor states sets in the mask are restored from the
> - * xsave area (See the comment in XFEATURE_MASK_INDEPENDENT for the details of
> - * independent supervisor feature). Besides the independent supervisor states, the
> - * legacy region and XSAVE header are also restored from the xsave area. The
> - * supervisor features in the XFEATURE_MASK_SUPERVISOR_SUPPORTED and
> - * XFEATURE_MASK_SUPERVISOR_UNSUPPORTED are not restored.
> + * Proper usage is to restore the state which was saved with
> + * xsaves() into @xstate.
> *
> - * The xsave area must be 64-bytes aligned.
> + * The feature mask must either be a subset of the independent features or
> + * a subset of the task->fpstate related features
> */
> -void copy_kernel_to_independent_supervisor(struct xregs_state *xstate, u64 mask)
> +void xrstors(struct xregs_state *xstate, u64 mask)
> {
> - u64 independent_mask = xfeatures_mask_independent() & mask;
> - u32 lmask, hmask;
> + u64 xchk;
> int err;
>
> - if (WARN_ON_FPU(!boot_cpu_has(X86_FEATURE_XSAVES)))
> + if (WARN_ON_FPU(!cpu_feature_enabled(X86_FEATURE_XSAVES)))
> return;
> + /*
> + * Validate that this is either a task->fpstate related component
> + * subset or an independent one.
> + */
> + if (mask & xfeatures_mask_independent())
> + xchk = ~xfeatures_mask_independent();
> + else
> + xchk = ~xfeatures_mask_all;
>
> - if (WARN_ON_FPU(!independent_mask))
> + if (WARN_ON_ONCE(!mask || mask & xchk))
> return;
>
> - lmask = independent_mask;
> - hmask = independent_mask >> 32;
> -
> - XSTATE_OP(XRSTORS, xstate, lmask, hmask, err);
> -
> - /* Should never fault when copying from a kernel buffer */
> - WARN_ON_FPU(err);
> + XSTATE_OP(XRSTORS, xstate, (u32)mask, (u32)(mask >> 32), err);
> + WARN_ON_ONCE(err);
> }
>
> #ifdef CONFIG_PROC_PID_ARCH_STATUS
>
Powered by blists - more mailing lists