Messages in this thread | | | From | Kees Cook <> | Date | Fri, 12 Aug 2016 10:36:21 -0700 | Subject | Re: [PATCH v3 50/51] x86/mm: move arch_within_stack_frames() to usercopy.c |
| |
On Fri, Aug 12, 2016 at 7:29 AM, Josh Poimboeuf <jpoimboe@redhat.com> 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").
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> > --- > 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.
> +/* > + * 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) > + */ > +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; > + > + 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; > + } > + return -1; > +} > +#endif /* CONFIG_HARDENED_USERCOPY */ > -- > 2.7.4 >
-Kees
-- Kees Cook Nexus Security
| |