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, 21 Jul 2017 08:24:52 -0500
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Andrey Ryabinin <ryabinin.a.a@...il.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"

On Fri, Jul 21, 2017 at 12:13:31PM +0300, Andrey Ryabinin wrote:
> > 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.
> 
> All I see is just bunch of reordering of independent instructions, like this:
> 
> -ffffffff81012760:      5b                      pop    %rbx
> -ffffffff81012761:      31 c0                   xor    %eax,%eax
> +ffffffff81012760:      31 c0                   xor    %eax,%eax
> +ffffffff81012762:      5b                      pop    %rbx
> 
> -ffffffff810c29ae:      48 83 c4 28             add    $0x28,%rsp
> -ffffffff810c29b2:      89 d8                   mov    %ebx,%eax
> +ffffffff810c29ae:      89 d8                   mov    %ebx,%eax
> +ffffffff810c29b0:      48 83 c4 28             add    $0x28,%rsp
> 
> I haven't noticed any single bad/harmful change. The size of .text remained the same. 

I compiled with -ffunction-sections to make the comparisons easier.  The
reordering is much more extreme than your example.  (This is with GCC 7,
btw).  And it's not just reordering of instructions.  It's control flow
changes as well.

Also, the text size grew a little:

      text	   data	     bss	     dec	    hex	filename
  10630602	8295074	16461824	35387500	21bf86c	vmlinux.before
  10634013	8295074	16461824	35390911	21c05bf	vmlinux.after

A small two-line change, which is supposed to be a noop, or at least
should only affect a small number of functions, but which instead
affects optimization decisions across the entire kernel, is actively
harmful IMO.

> And btw, arm/arm64 already use global current_stack_pointer just fine.

I wonder if they looked for the impact.

-- 
Josh