linux-kernel - Re: [PATCH v10 01/26] Documentation/x86: Add CET description

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: <58319765-891D-44B9-AF18-64492B01FF36@amacapital.net>
Date:   Mon, 18 May 2020 18:35:21 -0700
From:   Andy Lutomirski <luto@...capital.net>
To:     Dave Hansen <dave.hansen@...el.com>
Cc:     Yu-cheng Yu <yu-cheng.yu@...el.com>, x86@...nel.org,
        "H. Peter Anvin" <hpa@...or.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-mm@...ck.org,
        linux-arch@...r.kernel.org, linux-api@...r.kernel.org,
        Arnd Bergmann <arnd@...db.de>,
        Andy Lutomirski <luto@...nel.org>,
        Balbir Singh <bsingharora@...il.com>,
        Borislav Petkov <bp@...en8.de>,
        Cyrill Gorcunov <gorcunov@...il.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Eugene Syromiatnikov <esyr@...hat.com>,
        Florian Weimer <fweimer@...hat.com>,
        "H.J. Lu" <hjl.tools@...il.com>, Jann Horn <jannh@...gle.com>,
        Jonathan Corbet <corbet@....net>,
        Kees Cook <keescook@...omium.org>,
        Mike Kravetz <mike.kravetz@...cle.com>,
        Nadav Amit <nadav.amit@...il.com>,
        Oleg Nesterov <oleg@...hat.com>, Pavel Machek <pavel@....cz>,
        Peter Zijlstra <peterz@...radead.org>,
        Randy Dunlap <rdunlap@...radead.org>,
        "Ravi V. Shankar" <ravi.v.shankar@...el.com>,
        Vedvyas Shanbhogue <vedvyas.shanbhogue@...el.com>,
        Dave Martin <Dave.Martin@....com>,
        Weijiang Yang <weijiang.yang@...el.com>
Subject: Re: [PATCH v10 01/26] Documentation/x86: Add CET description



> On May 18, 2020, at 5:38 PM, Dave Hansen <dave.hansen@...el.com> wrote:
> 
> On 5/18/20 4:47 PM, Yu-cheng Yu wrote:
>>> On Fri, 2020-05-15 at 19:53 -0700, Yu-cheng Yu wrote:
>>> On Fri, 2020-05-15 at 16:56 -0700, Dave Hansen wrote:
>>>> On 5/15/20 4:29 PM, Yu-cheng Yu wrote:
>>>>> [...]
>>>>> I have run them with CET enabled.  All of them pass, except for the following:
>>>>> Sigreturn from 64-bit to 32-bit fails, because shadow stack is at a 64-bit
>>>>> address.  This is understandable.
>>>> [...]
>>>> One a separate topic: You ran the selftests and one failed.  This is a
>>>> *MASSIVE* warning sign.  It should minimally be described in your cover
>>>> letter, and accompanied by a fix to the test case.  It is absolutely
>>>> unacceptable to introduce a kernel feature that causes a test to fail.
>>>> You must either fix your kernel feature or you fix the test.
>>>> 
>>>> This code can not be accepted until this selftests issue is rectified.
>> The x86/sigreturn test constructs 32-bit ldt entries, and does sigreturn from
>> 64-bit to 32-bit context.  We do not have a way to construct a static 32-bit
>> shadow stack.
> 
> Why? What's the limiting factor?  Hardware architecture?  Something in
> the kernel?
> 
>> Why do we want that?  I think we can simply run the test with CET
>> disabled.
> 
> The sadistic parts of selftests/x86 come from real bugs.  Either bugs
> where the kernel fell over, or where behavior changed that broke apps.
> I'd suggest doing some research on where that particular test case came
> from.  Find the author of the test, look at the changelogs.
> 
> If this is something that a real app does, this is a problem.  If it's a
> sadistic test that Andy L added because it was an attack vector against
> the entry code, it's a different story.

There are quite a few tests that do these horrible things in there. IN my personal opinion, sigreturn.c is one of the most important tests we have — it does every horrible thing to the entry code that I thought of and that I could come up with a way of doing.  We have been saved from regressing many times by these tests.  CET, and especially the CPL0 version of CET, is its own set of entry horror, and we need to keep these tests working.

I assume the basic issue is that we call raise(), the context magically changes to 32-bit, but SSP has a 64-bit value, and horrors happen.  So I think two things need to happen:

1. Someone needs to document what happens when IRET tries to put a 64-bit value into SSP but CS is compat. Because Intel has plenty of history of doing colossally broken things here. IOW you could easily be hitting a hardware design problem, not a software issue per se.

2. The test needs to work. Assuming the hardware doesn’t do something utterly broken, either the 32-bit code needs to be adjusted to avoid any CALL
or RET, or you need to write a little raise_on_32bit_shstk() func that switches to an SSP that fits in 32 bits, calls raise(), and switches back.  From memory, I didn’t think there was a CALl or RET, so I’m guessing that SSP is getting truncated when we round trip through CPL3 compat mode and the result is that the kernel invoked the signal handler with the wrong SSP.  Whoops.

> 
> I don't personally know the background, but the changelogs can help you
> find the person that does.