linux-kernel - Re: [PATCH] x86/fpu: Warn only when CPU-provided sizes less than struct declaration

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]

Date:   Mon, 13 Jan 2020 16:54:52 +0700
From:   Suravee Suthikulpanit <suravee.suthikulpanit@....com>
To:     Dave Hansen <dave.hansen@...el.com>, linux-kernel@...r.kernel.org,
        x86@...nel.org
Cc:     tglx@...utronix.de, mingo@...hat.com, bp@...en8.de, hpa@...or.com,
        jon.grimm@....com,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Thomas Lendacky <Thomas.Lendacky@....com>
Subject: Re: [PATCH] x86/fpu: Warn only when CPU-provided sizes less than
 struct declaration

Dave,

On 12/12/19 1:52 PM, Suravee Suthikulpanit wrote:
> 
> On 12/11/2019 9:13 PM, Dave Hansen wrote:
>> On 12/10/19 9:24 PM, Suravee Suthikulpanit wrote:
>>> The value returned by ECX[1] indicates the alignment of state
>>> component i when the compacted format of the extended region of an
>>> XSAVE area is used (see Section 13.4.3). If ECX[1] returns 0, state
>>> component i is located immediately following the preceding state
>>> component; if ECX[1] returns 1, state component i is located on the
>>> next 64-byte boundary following the preceding state component.
>>
>> Essentially, if an implementation needs state alignment or (up to) 64
>> bytes of padding, it could use this existing architecture for it.
> 
> Let me check with the HW folks and get back to you on this.

I don't think the 64-byte aligned bit alone would help with the PKRU size mismatch warning in 
check_xstate_against_struct() that we are seeing.

Here is the description from the section 13.4.3, for the compacted format:

* Let j, 2 ≤ j < i, be the greatest value such that XCOMP_BV[j] = 1. Then locationI is
determined by the following values: locationJ; sizeJ, as enumerated in CPUID.(EAX=0DH,ECX=j):EAX;
and the value of alignI, as enumerated in CPUID.(EAX=0DH,ECX=i):ECX[1]:

   — If alignI = 0, locationI = locationJ + sizeJ. (This item implies that state component i is located immediately 
following the preceding state component whose bit is set in XCOMP_BV.)

   — If alignI = 1, locationI = ceiling(locationJ + sizeJ, 64). (This item implies that state component i is located on 
the next 64-byte boundary following the preceding state component whose bit is set in XCOMP_BV.)

According to the description above, since we need to add padding for PKRU state, if we set the aligned bit of the 
subsequent feature after PKRU state, the current Linux kernel would still generate the warning for PKRU since the 
check_xstate_against_struct() does not currently take into account of the align bit (which is only for compact mode).

It would also generate another warning in do_extra_xstate_size_checks() due to XSTATE_WARN_ON(paranoid_xstate_size != 
fpu_kernel_xstate_size) (see https://elixir.bootlin.com/linux/latest/source/arch/x86/kernel/fpu/xstate.c#L608). Note 
that the paranoid_xstate_size takes into account of the 64-byte alignment in the size calculation, but the 
fpu_kernel_xstate_size takes the size reported by CPUID function 0DH, sub-function 0 or 1. So, this logic might need to 
change as well.

As for the PKRU size, it should really be 4 bytes (regardless of the alignment) since that's the size of actual PKRU 
register, but it seems that the 4-byte padding in struct pkru_state was added to match the Intel's CPUID-report size of 
8. Now we also have AMD hardware reporting PKRU size of 32 (also mainly for padding). We shouldn't keep the same warning 
logic in check_xstate_against_struct().

Please let me know if I am still missing anything.

Thanks,
Suravee