linux-kernel - Re: [PATCH 3/9] x86/entry/32: fix the end of the stack for newly forked tasks

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 20 Sep 2016 21:10:55 -0400
From:   Brian Gerst <brgerst@...il.com>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...nel.org>,
        "H . Peter Anvin" <hpa@...or.com>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Andy Lutomirski <luto@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Nilay Vaish <nilayvaish@...il.com>
Subject: Re: [PATCH 3/9] x86/entry/32: fix the end of the stack for newly
 forked tasks

On Tue, Sep 20, 2016 at 4:02 PM, Josh Poimboeuf <jpoimboe@...hat.com> wrote:
> Thanks to all the recent x86 entry code refactoring, most tasks' kernel
> stacks start at the same offset right below their saved pt_regs,
> regardless of which syscall was used to enter the kernel.  That creates
> a nice convention which makes it straightforward to identify the end of
> the stack, which can be useful for the unwinder to verify the stack is
> sane.
>
> Calling schedule_tail() directly breaks that convention because its an
> asmlinkage function so its argument has to be pushed on the stack.  Add
> a wrapper which creates a proper "end of stack" frame header before the
> call.
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@...hat.com>
> ---
>  arch/x86/entry/entry_32.S | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
> index deef561..f0a7444 100644
> --- a/arch/x86/entry/entry_32.S
> +++ b/arch/x86/entry/entry_32.S
> @@ -44,6 +44,7 @@
>  #include <asm/alternative-asm.h>
>  #include <asm/asm.h>
>  #include <asm/smap.h>
> +#include <asm/frame.h>
>
>         .section .entry.text, "ax"
>
> @@ -237,6 +238,23 @@ ENTRY(__switch_to_asm)
>  END(__switch_to_asm)
>
>  /*
> + * The unwinder expects the last frame on the stack to always be at the same
> + * offset from the end of the page, which allows it to validate the stack.
> + * Calling schedule_tail() directly would break that convention because its an
> + * asmlinkage function so its argument has to be pushed on the stack.  This
> + * wrapper creates a proper "end of stack" frame header before the call.
> + */
> +ENTRY(schedule_tail_wrapper)
> +       FRAME_BEGIN
> +
> +       pushl   %eax
> +       call    schedule_tail
> +       popl    %eax
> +
> +       FRAME_END
> +       ret
> +ENDPROC(schedule_tail_wrapper)
> +/*
>   * A newly forked process directly context switches into this address.
>   *
>   * eax: prev task we switched from
> @@ -244,9 +262,7 @@ END(__switch_to_asm)
>   * edi: kernel thread arg
>   */
>  ENTRY(ret_from_fork)
> -       pushl   %eax
> -       call    schedule_tail
> -       popl    %eax
> +       call    schedule_tail_wrapper
>
>         testl   %ebx, %ebx
>         jnz     1f              /* kernel threads are uncommon */
> --
> 2.7.4
>

Dropping asmlinkage from schedule_tail() would be a better option if possible.

--
Brian Gerst