linux-kernel - Re: [PATCH 11/21] x86/dumpstack: Handle stack overflow on all stacks

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: <20171128181554.jl2qp7ca2tgfnuiq@treble>
Date:   Tue, 28 Nov 2017 12:15:54 -0600
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Andy Lutomirski <luto@...capital.net>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Ingo Molnar <mingo@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        "H . Peter Anvin" <hpa@...or.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Borislav Petkov <bp@...en8.de>
Subject: Re: [PATCH 11/21] x86/dumpstack: Handle stack overflow on all stacks

On Mon, Nov 27, 2017 at 09:29:26PM -0800, Andy Lutomirski wrote:
> On Mon, Nov 27, 2017 at 8:29 PM, Josh Poimboeuf <jpoimboe@...hat.com> wrote:
> > On Mon, Nov 27, 2017 at 11:26:30AM -0800, Linus Torvalds wrote:
> >> On Mon, Nov 27, 2017 at 2:45 AM, Ingo Molnar <mingo@...nel.org> wrote:
> >> > From: Andy Lutomirski <luto@...nel.org>
> >> >
> >> > We currently special-case stack overflow on the task stack.  We're
> >> > going to start putting special stacks in the fixmap with a custom
> >> > layout, so they'll have guard pages, too.  Teach the unwinder to be
> >> > able to unwind an overflow of any of the stacks.
> >>
> >> Why isn't this together with 01/21? The two cases seem to be entirely
> >> identical and fundamentally the same issue.
> >
> > Yeah, they probably do belong in the same patch.
> >
> >> In fact, maybe the whole "stack overflow" special cases should be in
> >> "get_stack_info()" itself, rather than be special-cased in the
> >> callers?
> >
> > I would be nervous about doing that.  Several of the get_stack_info()
> > callers rely on it being honest.
> >
> > In fact, looking deeper at the above patch, it doesn't seem convincingly
> > safe to me.  What if the adjacent page doesn't exist?  Then when the
> > oops dumping code dereferences the 'stack' variable, you get an oops in
> > your oops.

I was wrong here.  Too much thinking, not enough sleep.  Looking at it
today, the patch looks safe again.

The 'stack' variable is actually safe to dereference afterwards because
it gets updated with the safe aligned address.

> Isn't the oops dumping code supposed to dereference everything using a
> special safe function?

That's not what it does now.  The 'stack' pointer variable is
dereferenced normally, unless KASAN is enabled.  It relies on
get_stack_info() doing the safety check.

> Anyway, get_stack_info() wouldn't really be lying.  It would just be
> returning something where begin..end doesn't contain the requested
> pointer.

At the very least, it would be surprising.  I prefer the current
approach, it's much more explicit.

-- 
Josh