linux-kernel - Re: v4.10: kernel stack frame pointer .. has bad value (null)

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: <20170307182855.262ezbon2pm67qfd@treble>
Date:   Tue, 7 Mar 2017 12:28:55 -0600
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Andy Lutomirski <luto@...capital.net>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Pavel Machek <pavel@....cz>,
        kernel list <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Andrew Lutomirski <luto@...nel.org>,
        Borislav Petkov <bp@...en8.de>,
        Brian Gerst <brgerst@...il.com>,
        Denys Vlasenko <dvlasenk@...hat.com>,
        Peter Anvin <hpa@...or.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: v4.10: kernel stack frame pointer .. has bad value (null)

On Tue, Mar 07, 2017 at 09:59:44AM -0800, Andy Lutomirski wrote:
> On Tue, Mar 7, 2017 at 9:52 AM, Linus Torvalds
> <torvalds@...ux-foundation.org> wrote:
> > On Tue, Mar 7, 2017 at 9:38 AM, Josh Poimboeuf <jpoimboe@...hat.com> wrote:
> >>
> >> So I'm thinking we should have -maccumulate-outgoing-args always enabled
> >> on x86_32 just like we already do on x86_64.
> >
> > Ugh. I realize we have workarounds for bugs, but I think
> > -maccumulate-outgoing-args is nasty. It just generates worse code by
> > avoiding the much nicer push/pop sequences, afaik.

Yes, maybe the pushes/pops around a function call are a little easier to
read than movs.

But the -maccumulate-outgoing-args realignment prologue is a *lot* worse
for readability, IMO.

Also, the gcc documentation says -maccumulate-outgoing-args is
"generally beneficial for performance and size."

Not to mention the fact that -maccumulate-outgoing-args seems to already
be enabled in most cases anyway.  Having it uniformly enabled everywhere
makes it less confusing overall when the rare divergences are
encountered.  From looking at some of the changes related to
ADD_ACCUMULATE_OUTGOING_ARGS in arch/x86/Makefile_32.cpu, I can tell
that several others before me have stumbled into this prologue issue.

> > On x86-64 it's not such a big deal, because we pass the first six
> > arguments in registers anyway, so the arguments on the stack is a
> > fairly unusual special case.
> >
> > But on x86-32, we only have three argument registers, so this
> > braindamage is potentially worse.
> >
> > I guess we already do this in most situations due to the gcc bugs, but
> > I do think it's sad that we would do it for our _own_ bugs too.
> >
> 
> Is it our bug or a gcc bug?  I would have thought
> -fno-omit-frame-pointer meant that the call-frame-to-return-address
> offset should be constant and -fomit-frame-pointer meant "do
> whatever".

I don't think it's a gcc bug because it doesn't seem to violate frame
pointer conventions:

    pushl -0x4(%edi)	# copy return address
    push %ebp

The frame pointer and return address are still stored adjacently.  And
it normally allows unwinds to work fine.

The problem is the kernel unwinder's assumption that the last frame
pointer is at a certain address.  That assumption breaks with the DRAP
prologue.

> Also, maybe I'm missing something, but does gcc's code even allow the
> function to return sensibly?  It could do it by a nasty calculation
> involving backing out the old esp from edi, but that seems quite
> overcomplicated.

That's what it does:

    lea -0x8(%edi),%esp
    pop %edi
    ret

-- 
Josh