linux-kernel - Re: [PATCH v3 08/21] x86/fpu/xstate: Define the scope of the initial xstate data

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 8 Feb 2021 13:33:30 +0100
From:   Borislav Petkov <bp@...e.de>
To:     "Chang S. Bae" <chang.seok.bae@...el.com>
Cc:     luto@...nel.org, tglx@...utronix.de, mingo@...nel.org,
        x86@...nel.org, len.brown@...el.com, dave.hansen@...el.com,
        jing2.liu@...el.com, ravi.v.shankar@...el.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 08/21] x86/fpu/xstate: Define the scope of the initial
 xstate data

On Wed, Dec 23, 2020 at 07:57:04AM -0800, Chang S. Bae wrote:
> init_fpstate is used to record the initial xstate value for convenience

convenience?

> and covers all the states. But it is wasteful to cover large states all
> with trivial initial data.
> 
> Limit init_fpstate by clarifying its size and coverage, which are all but
> dynamic user states. The dynamic states are assumed to be large but having
> initial data with zeros.
> 
> No functional change until the kernel supports dynamic user states.

What does that mean?

This patch either makes no functional change or it does...

> Signed-off-by: Chang S. Bae <chang.seok.bae@...el.com>
> Reviewed-by: Len Brown <len.brown@...el.com>
> Cc: x86@...nel.org
> Cc: linux-kernel@...r.kernel.org
> ---
> Changes from v2:
> * Updated the changelog for clarification.
> * Updated the code comments.
> ---
>  arch/x86/include/asm/fpu/internal.h | 18 +++++++++++++++---
>  arch/x86/include/asm/fpu/xstate.h   |  1 +
>  arch/x86/kernel/fpu/core.c          |  4 ++--
>  arch/x86/kernel/fpu/xstate.c        |  4 ++--
>  4 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
> index 37ea5e37f21c..bbdd304719c6 100644
> --- a/arch/x86/include/asm/fpu/internal.h
> +++ b/arch/x86/include/asm/fpu/internal.h
> @@ -80,6 +80,18 @@ static __always_inline __pure bool use_fxsr(void)
>  
>  extern union fpregs_state init_fpstate;
>  
> +static inline u64 get_init_fpstate_mask(void)
> +{
> +	/* init_fpstate covers states in fpu->state. */
> +	return (xfeatures_mask_all & ~xfeatures_mask_user_dynamic);
> +}

If you're going to introduce such a helper, then use it everywhere in the code:

$ git grep "xfeatures_mask_all & ~xfeatures_mask_user_dynamic"
arch/x86/kernel/fpu/core.c:239: dst_fpu->state_mask = xfeatures_mask_all & ~xfeatures_mask_user_dynamic;
arch/x86/kernel/fpu/xstate.c:148:       else if (mask == (xfeatures_mask_all & ~xfeatures_mask_user_dynamic))
arch/x86/kernel/fpu/xstate.c:932:       current->thread.fpu.state_mask = (xfeatures_mask_all & ~xfeatures_mask_user_dynamic);

and if you do that, do that in a separate pre-patch which does only this
conversion.

> +static inline unsigned int get_init_fpstate_size(void)
> +{
> +	/* fpu->state size is aligned with the init_fpstate size. */
> +	return fpu_kernel_xstate_min_size;
> +}
> +
>  extern void fpstate_init(struct fpu *fpu);
>  #ifdef CONFIG_MATH_EMULATION
>  extern void fpstate_init_soft(struct swregs_state *soft);
> @@ -269,12 +281,12 @@ static inline void copy_fxregs_to_kernel(struct fpu *fpu)
>  		     : "memory")
>  
>  /*
> - * This function is called only during boot time when x86 caps are not set
> - * up and alternative can not be used yet.
> + * Use this function to dump the initial state, only during boot time when x86
> + * caps not set up and alternative not available yet.
>   */

What's the point of this change? Also, "dump"?!

>  static inline void copy_xregs_to_kernel_booting(struct xregs_state *xstate)
>  {
> -	u64 mask = xfeatures_mask_all;
> +	u64 mask = get_init_fpstate_mask();
>  	u32 lmask = mask;
>  	u32 hmask = mask >> 32;
>  	int err;
> diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
> index 379e8f8b8440..62f6583f34fa 100644
> --- a/arch/x86/include/asm/fpu/xstate.h
> +++ b/arch/x86/include/asm/fpu/xstate.h
> @@ -103,6 +103,7 @@ extern void __init update_regset_xstate_info(unsigned int size,
>  					     u64 xstate_mask);
>  
>  void *get_xsave_addr(struct fpu *fpu, int xfeature_nr);
> +unsigned int get_xstate_size(u64 mask);
>  int alloc_xstate_buffer(struct fpu *fpu, u64 mask);
>  void free_xstate_buffer(struct fpu *fpu);
>  
> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> index 6dafed34be4f..aad1a7102096 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -206,10 +206,10 @@ void fpstate_init(struct fpu *fpu)
>  		return;
>  	}
>  
> -	memset(state, 0, fpu_kernel_xstate_min_size);
> +	memset(state, 0, fpu ? get_xstate_size(fpu->state_mask) : get_init_fpstate_size());
>  
>  	if (static_cpu_has(X86_FEATURE_XSAVES))
> -		fpstate_init_xstate(&state->xsave, xfeatures_mask_all);
> +		fpstate_init_xstate(&state->xsave, fpu ? fpu->state_mask : get_init_fpstate_mask());

<---- newline here.

>  	if (static_cpu_has(X86_FEATURE_FXSR))
>  		fpstate_init_fxstate(&state->fxsave);
>  	else

...

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg