linux-kernel - Re: [RFC PATCH] x86/fpu: Don't unconditionally add XFEATURE_MASK

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20190426200509.3q5ngl44o64q2lg2@linutronix.de>
Date:   Fri, 26 Apr 2019 22:05:09 +0200
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     Dave Hansen <dave.hansen@...el.com>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org, jannh@...gle.com,
        riel@...riel.com, mingo@...hat.com, bp@...e.de, Jason@...c4.com,
        luto@...nel.org, tglx@...utronix.de, rkrcmar@...hat.com,
        mingo@...nel.org, hpa@...or.com, kvm@...r.kernel.org,
        pbonzini@...hat.com, kurt.kanzenbach@...utronix.de
Subject: Re: [RFC PATCH] x86/fpu: Don't unconditionally add
 XFEATURE_MASK_FPSSE on sigentry

On 2019-04-26 12:04:55 [-0700], Dave Hansen wrote:
> I'm looking at the code and having a bit of a hard time connecting it to
> what you're saying here.
> 
> We are in copy_fpstate_to_sigframe(), right?  Let's assume we are
> using_compacted_format().  If copy_fpregs_to_sigframe() fails to copy,
> we return immediately and never get to the save_xstate_epilog() code in
> question.
> 
> So, to even get to save_xstate_epilog(), we had to do a *successful*
> copy_fpstate_to_sigframe() which, on XSAVE systems will use
> copy_xregs_to_user() which already uses plain XSAVE (not XSAVEOPT).

We are in:

copy_fpstate_to_sigframe()
 |
 copy_fpregs_to_sigframe() fails.
 |
 using_compacted_format()
  |
  copy_xstate_to_user()
   |
   __copy_xstate_to_user() the xstate_header
   |
   for (i = 0; i < XFEATURE_MAX; i++)
    copy only XFEATURE_SSE + XFEATURE_YMM, other aren't set.
   |
   xfeatures_mxcsr_quirk() true, we copy
   |
   __copy_xstate_to_user() the xstate_fx_sw_bytes

Now. The FP part of the xsave has never been touched which means the
bytes are not initialized. The are is filled with random conten on user
stack. The saved xfeatures say only (XFEATURE_SSE | XFEATURE_YMM) so a
XRESTOR of that gives us exactly what we stored.

> save_xstate_epilog() goes and tries to set XFEATURE_MASK_FPSSE on this
> XSAVE (literally the XSAVE instruction) generated header.

Correct. But we never saved the "data" for XFEATURE_MASK_FP and, as noted
above, the content is random garbage on the stack.
Be setting XFEATURE_MASK_FPSSE we claim that suddenly that the FP part
of the XSAVE area is valid which is not the case.

> But, if we're dealing with header.xfeatures generated by an XSAVE with
> the RFBM=-1, I don't understand how header.xfeatures could ever *not*
> have XFEATURE_MASK_FPSSE set.  The only way would be if we had gotten
> here after using FXSAVE, but I believe that's impossible since those
> systems have use_xsave()==0.

Look at the code path above. XSAVES stored the state in task's
&fpu->state.xsave using XSAVES and we copied the *saved* bits so it
looks like XSAVE. XFEATURE_MASK_FP was not set, so we skipped that area.

> IOW, I think the patch is right, but I'm not sure I totally agree with
> the reasoning.

As I described in the path description: XSAVE (the fastpath, the code
before I started touching it) would also not set XFEATURE_MASK_FP in
xfeatures *but* would set FP area to its ini-state. Therefore the
unconditional OR of XFEATURE_MASK_FPSSE does not hurt because the FP
area was initialized to its initstate. The following restore would load
a valid FP state.

Sebastian