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

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

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@linutronix.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