linux-kernel - Re: [patch 04/31] x86/fpu: Restrict xsaves()/xrstors() to independent states

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]

Message-ID: <YWWagCMsVduf4yVn@zn.tnic>
Date:   Tue, 12 Oct 2021 16:24:00 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>, x86@...nel.org,
        "Chang S. Bae" <chang.seok.bae@...el.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Arjan van de Ven <arjan@...ux.intel.com>,
        kvm@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [patch 04/31] x86/fpu: Restrict xsaves()/xrstors() to
 independent states

On Tue, Oct 12, 2021 at 02:00:04AM +0200, Thomas Gleixner wrote:
> These interfaces are really only valid for features which are independently
> managed and not part of the task context state for various reasons.
> 
> Tighten the checks and adjust the misleading comments.
> 
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> ---
>  arch/x86/kernel/fpu/xstate.c |   14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -1182,13 +1182,9 @@ static bool validate_xsaves_xrstors(u64

I guess then change the name too, to:

validate_indep_xstate_components()

or so?

Then you don't need the comment below.

>  	if (WARN_ON_FPU(!cpu_feature_enabled(X86_FEATURE_XSAVES)))
>  		return false;
>  	/*
> -	 * Validate that this is either a task->fpstate related component
> -	 * subset or an independent one.
> +	 * Validate that this is a independent compoment.

WARNING: 'compoment' may be misspelled - perhaps 'component'?
#78: FILE: arch/x86/kernel/fpu/xstate.c:1185:
+        * Validate that this is a independent compoment.
                                               ^^^^^^^^^
>  	 */
> -	if (mask & xfeatures_mask_independent())
> -		xchk = ~xfeatures_mask_independent();
> -	else
> -		xchk = ~xfeatures_mask_all;
> +	xchk = ~xfeatures_mask_independent();
>  
>  	if (WARN_ON_ONCE(!mask || mask & xchk))
>  		return false;
> @@ -1206,8 +1202,7 @@ static bool validate_xsaves_xrstors(u64
>   * buffer should be zeroed otherwise a consecutive XRSTORS from that buffer
>   * can #GP.
>   *
> - * The feature mask must either be a subset of the independent features or
> - * a subset of the task->fpstate related features.
> + * The feature mask must be a subset of the independent features

End with a fullstop.

>   */
>  void xsaves(struct xregs_state *xstate, u64 mask)
>  {
> @@ -1231,8 +1226,7 @@ void xsaves(struct xregs_state *xstate,
>   * Proper usage is to restore the state which was saved with
>   * xsaves() into @xstate.
>   *
> - * The feature mask must either be a subset of the independent features or
> - * a subset of the task->fpstate related features.
> + * The feature mask must be a subset of the independent features

Ditto.

>   */
>  void xrstors(struct xregs_state *xstate, u64 mask)
>  {

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette