Re: [PATCH] x86/fpu: Avoid writing LBR bit to IA32_XSS unless supported

[Thomas Gleixner <tglx@linutronix.de>]

On Thu, Aug 08 2024 at 16:30, Mitchell Levy via wrote:
> From: Mitchell Levy <levymitchell0@gmail.com>
>
> When computing which xfeatures are available, make sure that LBR is only
> present if both LBR is supported in general, as well as by XSAVES.
>
> There are two distinct CPU features related to the use of XSAVES as it
> applies to LBR: whether LBR is itself supported (strictly speaking, I'm
> not sure that this is necessary to check though it's certainly a good
> sanity check), and whether XSAVES supports LBR (see sections 13.2 and
> 13.5.12 of the Intel 64 and IA-32 Architectures Software Developer's
> Manual, Volume 1). Currently, the LBR subsystem correctly checks both
> (see intel_pmu_arch_lbr_init), however the xstate initialization
> subsystem does not.
>
> When calculating what value to place in the IA32_XSS MSR,
> xfeatures_mask_independent only checks whether LBR support is present,
> not whether XSAVES supports LBR. If XSAVES does not support LBR, this
> write causes #GP, leaving the state of IA32_XSS unchanged (i.e., set to
> zero, as its not written with other values, and its default value is
> zero out of RESET per section 13.3 of the arch manual).
>
> Then, the next time XRSTORS is used to restore supervisor state, it will
> fail with #GP (because the RFBM has zero for all supervisor features,
> which does not match the XCOMP_BV field). In particular,
> XFEATURE_MASK_FPSTATE includes supervisor features, so setting up the FPU
> will cause a #GP. This results in a call to fpu_reset_from_exception_fixup,
> which by the same process results in another #GP. Eventually this causes
> the kernel to run out of stack space and #DF.

Cute.

> Fixes: d72c87018d00 ("x86/fpu/xstate: Move remaining xfeature helpers to core")

This is not the culprit/

> Cc: stable@vger.kernel.org
>
> Signed-off-by: Mitchell Levy <levymitchell0@gmail.com>
> ---
>  arch/x86/kernel/fpu/xstate.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h
> index 2ee0b9c53dcc..574d2c2ea227 100644
> --- a/arch/x86/kernel/fpu/xstate.h
> +++ b/arch/x86/kernel/fpu/xstate.h
> @@ -61,7 +61,8 @@ static inline u64 xfeatures_mask_supervisor(void)
>  
>  static inline u64 xfeatures_mask_independent(void)
>  {
> -	if (!cpu_feature_enabled(X86_FEATURE_ARCH_LBR))
> +	if (!cpu_feature_enabled(X86_FEATURE_ARCH_LBR) ||
> +	    (fpu_kernel_cfg.max_features & XFEATURE_MASK_LBR) != XFEATURE_MASK_LBR)

This is wrong because fpu_kernel_cfg.max_features never contains
XFEATURE_MASK_LBR. It only contains the bits which are managed by the
FPU subsystem.

Thanks,

        tglx