linux-kernel - Re: [patch V3 38/66] x86/fpu/xstate: Sanitize handling of independent features

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YNHKWyB9pRmw7v+K@zn.tnic>
Date:   Tue, 22 Jun 2021 13:32:43 +0200
From:   Borislav Petkov <bp@...e.de>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Andy Lutomirski <luto@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Tony Luck <tony.luck@...el.com>,
        Yu-cheng Yu <yu-cheng.yu@...el.com>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Kan Liang <kan.liang@...ux.intel.com>
Subject: Re: [patch V3 38/66] x86/fpu/xstate: Sanitize handling of
 independent features

On Fri, Jun 18, 2021 at 04:19:01PM +0200, Thomas Gleixner wrote:
> The copy functions for the independent features are horribly named and the
> supervisor and independent part is just overengineered.
> 
> The point is that the supplied mask has either to be a subset of the
> independent feature or a subset of the task->fpu.xstate managed features.

features

> Rewrite it so it checks check for invalid overlaps of these areas in the

s/ check//

> caller supplied feature mask. Rename it so it follows the new naming
> convention for these operations. Mop up the function documentation.
> 
> This allows to use that function for other purposes as well.
> 
> Suggested-by: Peter Zijlstra <peterz@...radead.org>
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> Cc: Kan Liang <kan.liang@...ux.intel.com>
> ---
> V3: Rename
> ---
>  arch/x86/events/intel/lbr.c       |    6 +-
>  arch/x86/include/asm/fpu/xstate.h |    5 +-
>  arch/x86/kernel/fpu/xstate.c      |   93 +++++++++++++++++++-------------------
>  3 files changed, 53 insertions(+), 51 deletions(-)

...

>  /**
> - * copy_independent_supervisor_to_kernel() - Save independent supervisor states to
> - *                                           an xsave area
> - * @xstate: A pointer to an xsave area
> - * @mask: Represent the independent supervisor features saved into the xsave area
> + * xsaves - Save selected components to a kernel xstate buffer
> + * @xstate:	Pointer to the buffer
> + * @mask:	Feature mask to select the components to save
>   *
> - * Only the independent supervisor states sets in the mask are saved into the xsave
> - * area (See the comment in XFEATURE_MASK_INDEPENDENT for the details of independent
> - * supervisor feature). Besides the independent supervisor states, the legacy
> - * region and XSAVE header are also saved into the xsave area. The supervisor
> - * features in the XFEATURE_MASK_SUPERVISOR_SUPPORTED and
> - * XFEATURE_MASK_SUPERVISOR_UNSUPPORTED are not saved.
> + * The @xstate buffer must be 64 byte aligned and correctly initialized as
> + * XSAVES does not write the full xstate header. Before first use the
> + * buffer should be zeroed otherwise a consecutive XRSTORS from that buffer
> + * can #GP.
>   *
> - * The xsave area must be 64-bytes aligned.
> + * The feature mask must either be a subset of the independent features or
> + * a subset of the task->fpstate related features
						    ^
						    Fullstop.

In the corresponding sentence for xrstors() too.

Btw, looking at how almost identical the two are, they're kinda begging
for a common worker function __x_handle_supervisor() or so which is
called by xsaves() and xrstors(). But maybe later.

With the above nitpicks addressed:

Reviewed-by: Borislav Petkov <bp@...e.de>

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg