Re: [PATCH] x86/fpu: Remove dynamic features from xcomp_bv for init_fpstate

["Chang S. Bae" <chang.seok.bae@intel.com>]

On 10/11/2022 3:24 PM, Dave Hansen wrote:
> From: Yuan Yao <yuan.yao@intel.com>
> 
> This was found a couple of months ago in a big old AMX
> backport.  But, it looks to be a problem in mainline too.
> Please let me know if this looks OK.  I'd also especially
> appreciate some testing from folks that have AMX hardware
> handy.
> 
> Builds and survives a quick boot test on non-AMX hardware.
> 
> --
> 
> == Background ==
> 
> 'init_fpstate' is a sort of template for all of the fpstates
> that come after it.  It is copied when new processes are
> execve()'d or XRSTOR'd to get fpregs into a known state.
> 
> That means that it represents the *starting* state for a
> process's fpstate which includes only the 'default' features.
> Dynamic features can, of course, be acquired later, but
> processes start with only default_features.
> 
> During boot the kernel decides whether all fpstates will be
> kept in the compacted or uncompacted format.  This choice is
> communicated to the hardware via the XCOMP_BV field in all
> XSAVE buffers, including 'init_fpstate'.
> 
> == Problem ==
> 
> But, the existing XCOMP_BV calculation is incorrect.  It uses
> the set of 'max_features', not the default features.
> 
> As a result, when XRSTOR operates on buffers derived from
> 'init_fpstate', it may attempt to "tickle" dynamic features which
> are at offsets for which there is no space allocated in the
> fpstate.
> 
> == Scope ==
> 
> This normally results in a relatively harmless out-of-bounds
> memory read.  It's harmless because it never gets consumed.  But,
> if the fpstate is next to some unmapped memory, this "tickle" can
> cause a page fault and an oops.
> 
> This only causes issues on systems when dynamic features are
> available and when an XSAVE buffer is next to uninitialized
> memory.  In other words, it only affects recent Intel server
> CPUs, and in relatively few memory locations.
> 
> Those two things are why it took relatively long to catch this.
> 
> == Solution ==
> 
> Use 'default_features' to establish the init_fpstate
> xcomp_bv value.  Reset individual fpstate xcomp_bv values
> when the rest of the fpstate is reset.
> 
> [ dhansen: add reset code from tglx, rewrites
> 	   commit message and comments ]
> 
> Fixes: 1c253ff2287f ("x86/fpu: Move xstate feature masks to fpu_*_cfg")
> Suggested-by: Dave Hansen <dave.hansen@intel.com>
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Yuan Yao <yuan.yao@intel.com>
> Cc: stable@vger.kernel.org
> ---
>   arch/x86/kernel/fpu/core.c   | 3 +++
>   arch/x86/kernel/fpu/xstate.c | 7 ++++++-
>   2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> index 3b28c5b25e12..4d64de34da12 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -526,6 +526,9 @@ static void __fpstate_reset(struct fpstate *fpstate, u64 xfd)
>   	fpstate->xfeatures	= fpu_kernel_cfg.default_features;
>   	fpstate->user_xfeatures	= fpu_user_cfg.default_features;
>   	fpstate->xfd		= xfd;
> +
> +	/* Ensure that xcomp_bv matches ->xfeatures */
> +	xstate_init_xcomp_bv(&fpstate->regs.xsave, fpstate->xfeatures);
>   }
>   
>   void fpstate_reset(struct fpu *fpu)
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index c8340156bfd2..f9f45610c72f 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -360,7 +360,12 @@ static void __init setup_init_fpu_buf(void)
>   
>   	print_xstate_features();
>   
> -	xstate_init_xcomp_bv(&init_fpstate.regs.xsave, fpu_kernel_cfg.max_features);
> +	/*
> +	 * 'init_fpstate' is sized for the default feature
> +	 * set without any of the dynamic features.
> +	 */
> +	xstate_init_xcomp_bv(&init_fpstate.regs.xsave,
> +			     fpu_kernel_cfg.default_features);

Along with this, perhaps this hunk needs to be updated as well [2]:

In arch/x86/kernel/fpu/init.c,

static void __init fpu__init_init_fpstate(void)
{
	/* Bring init_fpstate size and features up to date */
	init_fpstate.size		= fpu_kernel_cfg.max_size;
	init_fpstate.xfeatures		= fpu_kernel_cfg.max_features;
}

I attempted to fix the issue in the earlier posting [1]. But, mine looks 
to be clearly missing that __fpstate_reset() part.

Thanks,
Chang

[1]: 
https://lore.kernel.org/lkml/20220824191223.1248-1-chang.seok.bae@intel.c
[2]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/fpu/init.c#n213