Re: [PATCH v3 50/51] x86/mm: move arch_within_stack_frames() to usercopy.c

[Kees Cook]

On Fri, Aug 12, 2016 at 12:12 PM, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
> On Fri, Aug 12, 2016 at 10:36:21AM -0700, Kees Cook wrote:
>> On Fri, Aug 12, 2016 at 7:29 AM, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
>> > When I tried to port arch_within_stack_frames() to use the new unwinder,
>> > I got a nightmare include file "header soup" scenario when unwind.h was
>> > included from thread_info.h.  And anyway, I think thread_info.h isn't
>> > really an appropriate place for this function.  So move it to usercopy.c
>> > instead.
>> >
>> > Since it relies on its parent's stack pointer, and the function is no
>> > longer inlined, the arguments to the __builtin_frame_address() calls
>> > have been incremented.
>>
>> Cool, looks good (minor change noted below). This patch might be a
>> good place to drop this from mm/Makefile too:
>>
>> # Since __builtin_frame_address does work as used, disable the warning.
>> CFLAGS_usercopy.o += $(call cc-disable-warning, frame-address)
>>
>> Since frame-address warnings have been disabled globally now since
>> commit 124a3d88fa20 ("Disable "frame-address" warning").
>
> Ok, I'll do that with the next patch (51/51) which removes the
> __builtin_frame_address() calls.
>
>> > Signed-off-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
>> > ---
>> >  arch/x86/include/asm/thread_info.h | 46 ++++++++------------------------------
>> >  arch/x86/lib/usercopy.c            | 43 +++++++++++++++++++++++++++++++++++
>> >  2 files changed, 52 insertions(+), 37 deletions(-)
>> >
>> > diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
>> > index 8b7c8d8e..fd849e6 100644
>> > --- a/arch/x86/include/asm/thread_info.h
>> > +++ b/arch/x86/include/asm/thread_info.h
>> > @@ -176,49 +176,21 @@ static inline unsigned long current_stack_pointer(void)
>> >         return sp;
>> >  }
>> >
>> > -/*
>> > - * Walks up the stack frames to make sure that the specified object is
>> > - * entirely contained by a single stack frame.
>> > - *
>> > - * Returns:
>> > - *              1 if within a frame
>> > - *             -1 if placed across a frame boundary (or outside stack)
>> > - *              0 unable to determine (no frame pointers, etc)
>> > - */
>> > +#ifdef CONFIG_HARDENED_USERCOPY
>>
>> This ifdef shouldn't be needed: the arch_within_stack_frames wasn't
>> designed to depend on it.
>>
>> > +#ifdef CONFIG_FRAME_POINTER
>> > +int arch_within_stack_frames(const void * const stack,
>> > +                            const void * const stackend,
>> > +                            const void *obj, unsigned long len);
>> > +#else
>> >  static inline int arch_within_stack_frames(const void * const stack,
>> >                                            const void * const stackend,
>> >                                            const void *obj, unsigned long len)
>> >  {
>> > -#if defined(CONFIG_FRAME_POINTER)
>> > -       const void *frame = NULL;
>> > -       const void *oldframe;
>> > -
>> > -       oldframe = __builtin_frame_address(1);
>> > -       if (oldframe)
>> > -               frame = __builtin_frame_address(2);
>> > -       /*
>> > -        * 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;
>> > -       }
>> > -       return -1;
>> > -#else
>> >         return 0;
>> > -#endif
>> >  }
>> > +#endif /* CONFIG_FRAME_POINTER */
>> > +#endif /* CONFIG_HARDENED_USERCOPY */
>> > +
>> >
>> >  #else /* !__ASSEMBLY__ */
>> >
>> > diff --git a/arch/x86/lib/usercopy.c b/arch/x86/lib/usercopy.c
>> > index b490878..96ce151 100644
>> > --- a/arch/x86/lib/usercopy.c
>> > +++ b/arch/x86/lib/usercopy.c
>> > @@ -9,6 +9,7 @@
>> >
>> >  #include <asm/word-at-a-time.h>
>> >  #include <linux/sched.h>
>> > +#include <asm/unwind.h>
>> >
>> >  /*
>> >   * We rely on the nested NMI work to allow atomic faults from the NMI path; the
>> > @@ -34,3 +35,45 @@ copy_from_user_nmi(void *to, const void __user *from, unsigned long n)
>> >         return ret;
>> >  }
>> >  EXPORT_SYMBOL_GPL(copy_from_user_nmi);
>> > +
>> > +#ifdef CONFIG_HARDENED_USERCOPY
>>
>> Same thing: no need to check CONFIG_HARDENED_USERCOPY here: it should
>> be checking CONFIG_FRAME_POINTER instead.
>
> Now that this function is no longer inlined and is instead compiled in
> its own .c file, I was thinking that the tinyconfig folks would
> appreciate not growing the text size if there's no reason to do so.
> Keeping this #ifdef won't break anything, right?

Hrm, well, I guess not, but it means if anyone else wants to use it
they have to remove the ifdef. I guess I don't object that much. :P

> Also I moved the CONFIG_FRAME_POINTER check to the header file so it
> doesn't pollute the .c code.

Right, but if FRAME_POINTER=n and HARDENED_USERCOPY=y you'll get a
build error about it being both in the .h and the .c file, if I'm
reading that correctly.

-Kees

-- 
Kees Cook
Nexus Security