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: <D147640C-0248-4934-8F11-69706E0EE5CB@intel.com>
Date:   Tue, 29 Jun 2021 19:13:45 +0000
From:   "Bae, Chang Seok" <chang.seok.bae@...el.com>
To:     "Hansen, Dave" <dave.hansen@...el.com>
CC:     "Lutomirski, Andy" <luto@...nel.org>, 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 29, 2021, at 11:50, Hansen, Dave <dave.hansen@...el.com> wrote:
> On 6/29/21 11:35 AM, Bae, Chang Seok wrote:
>>        if (likely(use_xsave())) {
>> +               /*
>> +                * MSR IA32_XFD write follows after this XSAVE(S). So if a
>> +                * state component is in use, XFD should not be armed for
>> +                * current. But, for potential changes in the future,
>> +                * cross-check XINUSE and XFD values. If a XINUSE state
>> +                * is XFD-armed, the following XSAVE(S) does not save the
>> +                * state.
>> +                *
>> +                * Reference the shadow XFD value instead of reading the
>> +                * MSR.
>> +                */
>> +               if (xfd_capable() && boot_cpu_has(X86_FEATURE_XGETBV1)) {
>> +                       u64 current_xfd = (fpu->state_mask & xfd_capable()) ^ xfd_capable();
>> +
>> +                       WARN_ON_FPU(xgetbv(1) & current_xfd);
>> +               }
> 
> The code looks fine.  But, as usual, I hate the comment.  Maybe:
> 
> 		/*
> 		 * If XFD is armed for an xfeature, XSAVE* will not save
> 		 * its state.  Ensure XFD is clear for all features that
> 		 * are in use before XSAVE*.
> 		 */
> 
> BTW, the ->state_mask calculation is a little confusing to me.  I
> understand that fpu->state_mask shouldn't have any bits set that are
> unset in xgetbv(1).
> 
> This code seems to be asking the question: Are any dynamic features in
> their init state *and* XFD-armed?
> 
> Is it actually important to make sure that they are dynamic features?
> Is there *any* case where a feature (dynamic or not) can have XFD armed
> and be out of its init state?

In this AMX series, XFD is only used for the xstate buffer management. The
code is made in a such way that XFD and dynamic states are a bit coupled.

But I think XFD can be extended for other usages in the future. Then, yes.
(This warning is also for future code changes.)

So, reading the MSR is just simple and clean here, but it consumes cycles. Or,
a task may have a field for XFD value per se unless this conversion is
acceptable.

Thanks,
Chang