linux-kernel - Re: [PATCH] x86/orc: Don't bail on stack overflow

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: <20171126044115.tper4nvt47tsxr2j@treble>
Date:   Sat, 25 Nov 2017 22:41:15 -0600
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Andy Lutomirski <luto@...nel.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>, X86 ML <x86@...nel.org>,
        Borislav Petkov <bpetkov@...e.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Brian Gerst <brgerst@...il.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH] x86/orc: Don't bail on stack overflow

On Sat, Nov 25, 2017 at 08:25:12PM -0800, Andy Lutomirski wrote:
> On Sat, Nov 25, 2017 at 6:40 PM, Josh Poimboeuf <jpoimboe@...hat.com> wrote:
> > On Sat, Nov 25, 2017 at 04:16:23PM -0800, Andy Lutomirski wrote:
> >> Can you send me whatever config and exact commit hash generated this?
> >> I can try to figure out why it failed.
> >
> > Sorry, I've been traveling.  I just got some time to take a look at
> > this.  I think there are at least two unwinder issues here:
> >
> > - It doesn't deal gracefully with the case where the stack overflows and
> >   the stack pointer itself isn't on a valid stack but the
> >   to-be-dereferenced data *is*.
> >
> > - The oops dump code doesn't know how to print partial pt_regs, for the
> >   case where if we get an interrupt/exception in *early* entry code
> >   before the full pt_regs have been saved.
> >
> > (Andy, I'm not quite sure about your patch, and whether it's still
> > needed after these patches.  I'll need to look at it later when I have
> > more time.)
> 
> I haven't tested yet, but I think my patch is probably still needed.
> The issue I fixed is that unwind_start() would bail out early if sp
> was below the stack.  Also:

Makes sense, maybe both are needed.  Your patch deals with a bad SP at
the beginning and mine deals with a bad SP in the middle.

> > -static bool stack_access_ok(struct unwind_state *state, unsigned long addr,
> > +static bool stack_access_ok(struct unwind_state *state, unsigned long _addr,
> >                             size_t len)
> >  {
> >         struct stack_info *info = &state->stack_info;
> > +       void *addr = (void *)_addr;
> >
> > -       /*
> > -        * If the address isn't on the current stack, switch to the next one.
> > -        *
> > -        * We may have to traverse multiple stacks to deal with the possibility
> > -        * that info->next_sp could point to an empty stack and the address
> > -        * could be on a subsequent stack.
> > -        */
> > -       while (!on_stack(info, (void *)addr, len))
> > -               if (get_stack_info(info->next_sp, state->task, info,
> > -                                  &state->stack_mask))
> > -                       return false;
> > +       if (!on_stack(info, addr, len) &&
> > +           (get_stack_info(addr, state->task, info, &state->stack_mask)))
> > +               return false;
> >
> >         return true;
> >  }
> 
> This looks odd to me before and after.  Shouldn't this be side-effect
> free?  That is, shouldn't it create a copy of info and stack_mask and
> point that to get_stack_info() rather than allowing get_stack_info()
> to modify the unwind state?

I think the side effects are ok, but maybe stack_access_ok() should be
renamed to make it clearer that it has side effects.

-- 
Josh