Re: [PATCH v2 2/8] x86/fpu/xstate: Separate user and supervisor xfeatures mask

[Borislav Petkov <bp@alien8.de>]

On Tue, Jan 21, 2020 at 12:18:37PM -0800, Yu-cheng Yu wrote:
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index 400a05e1c1c5..7c7f3efa3c57 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -254,11 +254,14 @@ static int copy_user_to_fpregs_zeroing(void __user *buf, u64 xbv, int fx_only)
>  {
>  	if (use_xsave()) {
>  		if (fx_only) {
> -			u64 init_bv = xfeatures_mask & ~XFEATURE_MASK_FPSSE;
> +			u64 init_bv;
> +
> +			init_bv = xfeatures_mask_user() & ~XFEATURE_MASK_FPSSE;
>  			copy_kernel_to_xregs(&init_fpstate.xsave, init_bv);
>  			return copy_user_to_fxregs(buf);
>  		} else {
> -			u64 init_bv = xfeatures_mask & ~xbv;
> +			u64 init_bv = xfeatures_mask_user() & ~xbv;
> +
>  			if (unlikely(init_bv))
>  				copy_kernel_to_xregs(&init_fpstate.xsave, init_bv);
>  			return copy_user_to_xregs(buf, xbv);

If you don't want to overflow 80 cols here in the fx_only() case, you can do:

---
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 7c7f3efa3c57..d7e94677cd31 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -252,15 +252,16 @@ sanitize_restored_xstate(union fpregs_state *state,
  */
 static int copy_user_to_fpregs_zeroing(void __user *buf, u64 xbv, int fx_only)
 {
+	u64 init_bv;
+
 	if (use_xsave()) {
 		if (fx_only) {
-			u64 init_bv;
-
 			init_bv = xfeatures_mask_user() & ~XFEATURE_MASK_FPSSE;
+
 			copy_kernel_to_xregs(&init_fpstate.xsave, init_bv);
 			return copy_user_to_fxregs(buf);
 		} else {
-			u64 init_bv = xfeatures_mask_user() & ~xbv;
+			init_bv = xfeatures_mask_user() & ~xbv;
 
 			if (unlikely(init_bv))
 				copy_kernel_to_xregs(&init_fpstate.xsave, init_bv);

...

> @@ -210,19 +211,24 @@ void fpstate_sanitize_xstate(struct fpu *fpu)
>   */
>  void fpu__init_cpu_xstate(void)
>  {
> -	if (!boot_cpu_has(X86_FEATURE_XSAVE) || !xfeatures_mask)
> +	if (!boot_cpu_has(X86_FEATURE_XSAVE) || !xfeatures_mask_all)
>  		return;
>  	/*
>  	 * Unsupported supervisor xstates should not be found in
>  	 * the xfeatures mask.
>  	 */
> -	WARN_ONCE((xfeatures_mask & UNSUPPORTED_XFEATURES_MASK_SUPERVISOR),
> +	WARN_ONCE((xfeatures_mask_all & UNSUPPORTED_XFEATURES_MASK_SUPERVISOR),
>  		  "x86/fpu: Found unsupported supervisor xstates.\n");

Let's say which to ease debugging:

	u64 unsup_bits = xfeatures_mask_all & UNSUPPORTED_XFEATURES_MASK_SUPERVISOR;

	...

	WARN_ONCE(unsup_bits, "x86/fpu: Found unsupported supervisor xstates: 0x%llx\n",
		  unsup_bits);


>  
> -	xfeatures_mask &= ~UNSUPPORTED_XFEATURES_MASK_SUPERVISOR;
> +	xfeatures_mask_all &= ~UNSUPPORTED_XFEATURES_MASK_SUPERVISOR;
>  
>  	cr4_set_bits(X86_CR4_OSXSAVE);
> -	xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask);

<---- newline here.

> +	/*
> +	 * XCR_XFEATURE_ENABLED_MASK (aka. XCR0) sets user features
> +	 * managed by XSAVE{C, OPT, S} and XRSTOR{S}.  Only XSAVE user
> +	 * states can be set here.
> +	 */
> +	xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask_user());
>  }
>  
>  /*
> @@ -232,7 +238,7 @@ void fpu__init_cpu_xstate(void)
>   */
>  static int xfeature_enabled(enum xfeature xfeature)

static bool

>  {
> -	return !!(xfeatures_mask & (1UL << xfeature));
> +	return !!(xfeatures_mask_all & (1UL << xfeature));

	return xfeatures_mask_all & BIT(xfeature);

while at it.

>  }
>  
>  /*
> @@ -419,7 +425,7 @@ static void __init setup_init_fpu_buf(void)
>  
>  	if (boot_cpu_has(X86_FEATURE_XSAVES))
>  		init_fpstate.xsave.header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT |
> -						     xfeatures_mask;
> +						     xfeatures_mask_all;
>  
>  	/*
>  	 * Init all the features state with header.xfeatures being 0x0
> @@ -479,7 +485,7 @@ int using_compacted_format(void)
>  int validate_xstate_header(const struct xstate_header *hdr)
>  {
>  	/* No unknown or supervisor features may be set */
> -	if (hdr->xfeatures & ~(xfeatures_mask & SUPPORTED_XFEATURES_MASK_USER))
> +	if (hdr->xfeatures & ~xfeatures_mask_user())
>  		return -EINVAL;
>  
>  	/* Userspace must use the uncompacted format */
> @@ -614,7 +620,7 @@ static void do_extra_xstate_size_checks(void)
>  
>  
>  /*
> - * Get total size of enabled xstates in XCR0/xfeatures_mask.
> + * Get total size of enabled xstates in XCR0 | IA32_XSS.
>   *
>   * Note the SDM's wording here.  "sub-function 0" only enumerates
>   * the size of the *user* states.  If we use it to size a buffer
> @@ -704,7 +710,7 @@ static int __init init_xstate_size(void)
>   */
>  static void fpu__init_disable_system_xstate(void)
>  {
> -	xfeatures_mask = 0;
> +	xfeatures_mask_all = 0;
>  	cr4_clear_bits(X86_CR4_OSXSAVE);
>  	setup_clear_cpu_cap(X86_FEATURE_XSAVE);
>  }
> @@ -739,16 +745,22 @@ void __init fpu__init_system_xstate(void)
>  		return;
>  	}
>  
> +	/*
> +	 * Find user xstates supported by the processor.
> +	 */
>  	cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx);
> -	xfeatures_mask = eax + ((u64)edx << 32);
> +	xfeatures_mask_all = eax + ((u64)edx << 32);
> +
> +	/* Place supervisor features in xfeatures_mask_all here */
>  

Superfluous newline.

> -	if ((xfeatures_mask & XFEATURE_MASK_FPSSE) != XFEATURE_MASK_FPSSE) {
> +	if ((xfeatures_mask_user() & XFEATURE_MASK_FPSSE) != XFEATURE_MASK_FPSSE) {
>  		/*
>  		 * This indicates that something really unexpected happened
>  		 * with the enumeration.  Disable XSAVE and try to continue
>  		 * booting without it.  This is too early to BUG().
>  		 */
> -		pr_err("x86/fpu: FP/SSE not present amongst the CPU's xstate features: 0x%llx.\n", xfeatures_mask);
> +		pr_err("x86/fpu: FP/SSE not present amongst the CPU's xstate features: 0x%llx.\n",
> +		       xfeatures_mask_all);
>  		goto out_disable;
>  	}

...

-- 
Regards/Gruss,
    Boris.

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