lkml.org 
[lkml]   [2022]   [Oct]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] x86/fpu: Remove dynamic features from xcomp_bv for init_fpstate
From
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

\
 
 \ /
  Last update: 2022-10-12 00:48    [W:0.132 / U:0.728 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site