linux-kernel - Re: [PATCH v5 14/28] x86/fpu/xstate: Prevent unauthorised use of dynamic user state

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: <36D0486A-D955-4C32-941A-A2A4985A450C@intel.com>
Date:   Wed, 16 Jun 2021 18:47:29 +0000
From:   "Bae, Chang Seok" <chang.seok.bae@...el.com>
To:     Andy Lutomirski <luto@...nel.org>,
        "Hansen, Dave" <dave.hansen@...el.com>
CC:     Borislav Petkov <bp@...e.de>, Thomas Gleixner <tglx@...utronix.de>,
        "Ingo Molnar" <mingo@...nel.org>, X86 ML <x86@...nel.org>,
        "Brown, Len" <len.brown@...el.com>,
        "Liu, Jing2" <jing2.liu@...el.com>,
        "Shankar, Ravi V" <ravi.v.shankar@...el.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 14/28] x86/fpu/xstate: Prevent unauthorised use of
 dynamic user state

On Jun 16, 2021, at 11:12, Andy Lutomirski <luto@...nel.org> wrote:
> On 6/16/21 9:27 AM, Dave Hansen wrote:
>> On 5/23/21 12:32 PM, Chang S. Bae wrote:
>>> @@ -571,6 +612,8 @@ static inline void switch_fpu_finish(struct fpu *new_fpu)
>>> 
>>> 	set_thread_flag(TIF_NEED_FPU_LOAD);
>>> 
>>> +	xfd_switch(old_fpu, new_fpu);
>> 
>> This seems fragile.
>> 
>> XSAVE* will decline to write out state for feature i when XFD[i]=1 and
>> will instead write out the init state.  That means that, at this point,
>> we switch XFD and yet leave state for feature i in place.
>> 
>> That means this *MUST* come before any copy_fpregs_to_fpstate() occurs.
>> 
>> Could we please add some FPU_WARN_ON() checks to ensure that no XSAVE*
>> is ever performed with XINUSE=1 *and* XFD armed?

So, xfd_switch() is after copy_fpregs_to_fpstate():
 
__switch_to()
--> switch_fpu_prepare() --> copy_fpregs_to_fpstate()
--> switch_fpu_finish() --> xfd_switch()
 
I don't see someone intentionally flip this order.

Reading XINUSE via XGETBV is cheap but not free. I don't know spending a
hundred cycles for this WARN is big deal but this is one of the most
performance-critical paths.

> Wait, really?  I somehow thought that XSAVE* would write out the actual
> state even if XFD=1.

No, in section 3.2.6 Extended Feature Disable (XFD) of [1]:

    "If XSAVE, XSAVEC, XSAVEOPT, or XSAVES is saving the state component i,
     the instruction does not generate #NM when XCR0[i] = IA32_XFD[i] = 1;
     instead, it saves bit i of XSTATE_BV field of the XSAVE header as 0 (
     indicating that the state component is in its initialized state). With
     the exception of XSAVE, no data is saved for the state component (XSAVE
     saves the initial value of the state component; for XTILEDATA, this is
     all zeroes)."

> This seems like it's asking for some kind of bug.

[1]: Intel Architecture Instruction Set Extensions Programming Reference, https://software.intel.com/content/www/us/en/develop/download/intel-architecture-instruction-set-extensions-programming-reference.html