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:   Fri, 28 Jul 2017 17:38:52 -0700
From:   Matthias Kaehlcke <mka@...omium.org>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     Andrey Ryabinin <ryabinin.a.a@...il.com>,
        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"

El Thu, Jul 20, 2017 at 03:56:52PM -0500 Josh Poimboeuf ha dit:

> On Thu, Jul 20, 2017 at 06:30:24PM +0300, Andrey Ryabinin wrote:
> > 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
> 
> Thanks, that clears up a lot of the confusion for me.
> 
> Still, unfortunately, I don't think that's going to work for GCC.
> Changing the '__sp' register variable to global in the header file
> causes it to make a *bunch* of changes across the kernel, even in
> functions which don't do inline asm.  It seems to be disabling some
> optimizations across the board.
> 
> I do have another idea, which is to replace all uses of
> 
>   asm(" ... call foo ... " : outputs : inputs : clobbers);
> 
> with a new ASM_CALL macro:
> 
>   ASM_CALL(" ... call foo ... ", outputs, inputs, clobbers);
> 
> Then the compiler differences can be abstracted out, with GCC adding
> "sp" as an output constraint and clang doing nothing (for now).

The idea sounds interesting, however I see two issues with ASM_CALL():

Inline assembly expects the individual elements of outputs, inputs and
clobbers to be comma separated, and so does the macro for it's
parameters.

The assembler template can refer to the position of output and input
operands, adding "sp" as output changes the positions of the inputs
wrt to the clang version.

Not sure how to move forward from here. Not even using an ugly #ifdef
seems to be a halfway reasonable solution, since get_user() isn't the
only place using this construct and #ifdefs would result in highly
redundant macros in multiple places.