linux-kernel - Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in get

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:   Thu, 20 Jul 2017 18:30:24 +0300
From:   Andrey Ryabinin <ryabinin.a.a@...il.com>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     Matthias Kaehlcke <mka@...omium.org>,
        Chris J Arges <chris.j.arges@...onical.com>,
        Borislav Petkov <bp@...e.de>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H . Peter Anvin" <hpa@...or.com>,
        "x86@...nel.org" <x86@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Douglas Anderson <dianders@...omium.org>,
        Michael Davidson <md@...gle.com>,
        Greg Hackmann <ghackmann@...gle.com>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Stephen Hines <srhines@...gle.com>,
        Kees Cook <keescook@...omium.org>,
        Arnd Bergmann <arnd@...db.de>,
        Bernhard Rosenkränzer 
        <Bernhard.Rosenkranzer@...aro.org>
Subject: Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in
 get_user() inline asm"

2017-07-20 18:18 GMT+03:00 Josh Poimboeuf <jpoimboe@...hat.com>:
> On Thu, Jul 20, 2017 at 01:01:39PM +0300, Andrey Ryabinin wrote:
>> 2017-07-19 20:46 GMT+03:00 Josh Poimboeuf <jpoimboe@...hat.com>:
>>
>> >
>> > After doing some testing, I don't think this approach is going to work
>> > after all.  In addition to forcing the stack frame, it also causes GCC
>> > to add an unnecessary extra instruction to the epilogue of each affected
>> > function:
>> >
>> >   lea    -0x10(%rbp),%rsp
>> >
>> > We shouldn't be inserting extra instructions like that.  I also don't
>> > think the other suggestion of turning the '__sp' register variable into
>> > a global variable is a very good solution either, as that just wastes
>> > memory for no reason.
>> >
>>
>> Wastes memory? How is that wastes memory? That doesn't make any sense.
>
> Yes, you're right, that doesn't make any sense.  I think I was trying to
> wrap my head around what it means to have a global register variable --
> in a header file no less -- and why clang would treat that differently
> than a local register variable.
>

FWIW bellow is my understanding of what's going on.

It seems clang treats local named register almost the same as ordinary
local variables.
The only difference is that before reading the register variable clang
puts variable's value into the specified register.

So clang just assigns stack slot for the variable __sp where it's
going to keep variable's value.
But since __sp is unitialized (we haven't assign anything to it), the
value of the __sp is some garbage from stack.
inline asm specifies __sp as input, so clang assumes that it have to
load __sp into 'rsp' because inline asm is going to use
it. And it just loads garbage from stack into 'rsp'

In fact, such behavior (I mean storing the value on stack and loading
into reg before the use) is very useful.
Clang's behavior allows to keep the value assigned to the
call-clobbered register across the function calls.

Unlike clang, gcc assigns value to the register right away and doesn't
store the value anywhere else. So if the reg is
call clobbered register you have to be absolutely sure that there is
no subsequent function call that might clobber the register.

E.g. see some real examples
https://patchwork.kernel.org/patch/4111971/ or 98d4ded60bda("msm: scm:
Fix improper register assignment").
These bugs shouldn't happen with clang.

But the global named register works slightly differently in clang. For
the global, the value is just the value of the register itself,
whatever it is. Read/write from global named register is just like
direct read/write to the register