Re: [PATCH v4 54/57] x86/mm: convert arch_within_stack_frames() to use the new unwinder

[Kees Cook]

On Thu, Aug 18, 2016 at 6:06 AM, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> 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@xxxxxxxxxx>

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