linux-kernel - Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack

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: <CAGXu5jKeL41AiP_u6TmzYa5AE+i6wJGvPmoiyaVU_d7U-UX-ag@mail.gmail.com>
Date:   Fri, 19 Aug 2016 11:27:18 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...nel.org>,
        "H . Peter Anvin" <hpa@...or.com>,
        "x86@...nel.org" <x86@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Andy Lutomirski <luto@...capital.net>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Brian Gerst <brgerst@...il.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Frederic Weisbecker <fweisbec@...il.com>,
        Byungchul Park <byungchul.park@....com>,
        Nilay Vaish <nilayvaish@...il.com>
Subject: Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to
 use the new unwinder

On Thu, Aug 18, 2016 at 6:06 AM, Josh Poimboeuf <jpoimboe@...hat.com> wrote:
> Convert arch_within_stack_frames() to use the new unwinder.
>
> This also changes some existing behavior:
>
> - Skip checking of pt_regs frames.
> - Warn if it can't reach the grandparent's stack frame.
> - Warn if it doesn't unwind to the end of the stack.
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@...hat.com>

All the stuff touching usercopy looks good to me. One question,
though, in looking through the unwinder. It seems like it's much more
complex than just the frame-hopping that the old
arch_within_stack_frames() did, but I'm curious to hear what you think
about its performance. We'll be calling this with every usercopy that
touches the stack, so I'd like to be able to estimate the performance
impact of this replacement...

-Kees

> ---
>  arch/x86/lib/usercopy.c | 44 ++++++++++++++++++++++++++++----------------
>  1 file changed, 28 insertions(+), 16 deletions(-)
>
> diff --git a/arch/x86/lib/usercopy.c b/arch/x86/lib/usercopy.c
> index 2492fa7..8fe0a9c 100644
> --- a/arch/x86/lib/usercopy.c
> +++ b/arch/x86/lib/usercopy.c
> @@ -50,30 +50,42 @@ int arch_within_stack_frames(const void * const stack,
>                              const void * const stackend,
>                              const void *obj, unsigned long len)
>  {
> -       const void *frame = NULL;
> -       const void *oldframe;
> +       struct unwind_state state;
> +       const void *frame, *frame_end;
> +
> +       /*
> +        * Start at the end of our grandparent's frame (beginning of
> +        * great-grandparent's frame).
> +        */
> +       unwind_start(&state, current, NULL, NULL);
> +       if (WARN_ON_ONCE(!unwind_next_frame(&state) ||
> +                        !unwind_next_frame(&state)))
> +               return 0;
> +       frame = unwind_get_stack_ptr(&state);
>
> -       oldframe = __builtin_frame_address(2);
> -       if (oldframe)
> -               frame = __builtin_frame_address(3);
>         /*
>          * low ----------------------------------------------> high
>          * [saved bp][saved ip][args][local vars][saved bp][saved ip]
>          *                     ^----------------^
>          *               allow copies only within here
>          */
> -       while (stack <= frame && frame < stackend) {
> -               /*
> -                * If obj + len extends past the last frame, this
> -                * check won't pass and the next frame will be 0,
> -                * causing us to bail out and correctly report
> -                * the copy as invalid.
> -                */
> -               if (obj + len <= frame)
> -                       return obj >= oldframe + 2 * sizeof(void *) ? 1 : -1;
> -               oldframe = frame;
> -               frame = *(const void * const *)frame;
> +       frame += 2*sizeof(long);
> +
> +       while (unwind_next_frame(&state)) {
> +               frame_end = unwind_get_stack_ptr(&state);
> +
> +               /* skip checking of pt_regs frames */
> +               if (!unwind_get_entry_regs(&state) &&
> +                   obj >= frame && obj + len <= frame_end)
> +                       return 1;
> +
> +               frame = frame_end + 2*sizeof(long);
>         }
> +
> +       /* make sure the unwinder reached the end of the task stack */
> +       if (WARN_ON_ONCE(frame != (void *)task_pt_regs(current)))
> +               return 0;
> +
>         return -1;
>  }
>  #endif /* CONFIG_HARDENED_USERCOPY && CONFIG_FRAME_POINTER */
> --
> 2.7.4
>



-- 
Kees Cook
Nexus Security