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 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:   Sat, 25 Nov 2017 20:25:12 -0800
From:   Andy Lutomirski <luto@...nel.org>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     Andy Lutomirski <luto@...nel.org>,
        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 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:

> -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?