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]

Message-ID: <7a8fe748-2c57-295a-e6ed-8969c41462aa@amd.com>
Date:   Wed, 11 Dec 2019 12:24:42 +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/6/19 10:28 PM, Dave Hansen wrote:
> On 12/6/19 12:14 AM, Suravee Suthikulpanit wrote:
>> On 12/4/19 12:27 AM, Dave Hansen wrote:
>>> On 12/3/19 1:01 AM, Suravee Suthikulpanit wrote:
>>>> The current XCHECK_SZ macro warns if the XFEATURE size reported
>>>> by CPUID does not match the size of kernel structure. However, depending
>>>> on the hardware implementation, CPUID can report the XSAVE state size
>>>> larger than the size of C structures defined for each of the XSAVE state
>>>> due to padding.
>>>
>>> We have existing architecture for padding.  See xfeature_is_aligned(),
>>> for instance.  Are you saying that there are implementations out there
>>> that do padding which is not otherwise enumerated and that they do it
>>> within the size of the enumerated stat
>> Yes, the implementation includes the padding size within the size of
>> the enumerated state. This results in the reported size larger than
>> the amount needed by the feature.

Actually, please allow me clarify my understanding for this part.

When you referred to "the existing architecture for padding", IIUC,
that's the XSAVE state size, offset, and alignment of each extended
feature reported by the CPUID Fn 0Dh E[A|B|C]X. By "padding", do you
mean the additional area included as part of alignment?

> I don't think we've ever had XSAVE state that differed in size between
> implementations.  This kind of thing ensures that we can't have any
> statically-defined inspection into the XSAVE state.
>
> It also increases the amount of blind trust that we have in the CPU
> implementations.  However, those warnings were specifically added at
> Ingo's behest (IIRC) to reduce our blind trust in the CPU.
> 

I am not quite sure what you meant by "statically-defined inspection" and
"blind trust".

>>>> Such case should be safe and should not need to generate warning
>>>> message.
>>>
>>> I've seen these error messages trip before, but only on pre-production
>>> processors with goofy microcode.  I'd be really suspicious that this is
>>> just papering over a processor issue.  Or, that perhaps the compacted
>>> form works but the standard form is broken somehow.
>>
>> I have verified with the HW folks and the have confirmed that this is
>> to be expected.
> 
>  From a review perspective, I'd really appreciate being able to have a
> concrete discussion about exactly which state this is and exactly how
> much padding we are talking about and *why* the existing padding
> architecture can't be used.  I'd also want guarantees about this state
> not getting used for any real state, *ever*.

Here is the warning message:

     ------------[ cut here ]------------
     XFEATURE_PKRU: struct is 8 bytes, cpu state 32 bytes
     WARNING: CPU: 0 PID: 0 at arch/x86/kernel/fpu/xstate.c:558 fpu__init_system_xstate+0x2b5/0x8ea
     .......

In this case, the feature is PKRU (feature enum 9), and CPUID 0Dh instruction reports
eax:0x20 (size), ebx:0x340 (offset), ecx:0(aligned). The warning is due to the 32-byte size
is more than the 8 bytes required for struct pkru_state.

What I have been told (by HW folks) is that the hardware reports 32 bytes for PKRU feature
to account for additional padding (24 bytes) required to maintain offset for the XSAVE data
in compact form where:

     offset of the next component = offset of current component +
                                    size of current component

In this case, the hardware adds the padding needed before the offset of the subsequent
feature into the PKRU xsave state size.

Please correct me if I am wrong, but I believe this is similar to the case
mentioned in the commit ef78f2a4bf84 ('x86/fpu: Check CPU-provided sizes against struct declarations'),
where it mentions inconsistency b/w the MPX 'bndcsr' state and the C structures.

However, my point is that it should not need to warn if the xsave size is greater than
the size of the C structure.

Thanks,
Suravee