Re: [patch V3 38/66] x86/fpu/xstate: Sanitize handling of independent features

["Liang, Kan" <kan.liang@linux.intel.com>]

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@infradead.org>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Kan Liang <kan.liang@linux.intel.com>
> ---


I have tested the LBR Xsave feature. Everything looks good.

Tested-by: Kan Liang <kan.liang@linux.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
>