Messages in this thread | | | From | Kees Cook <> | Date | Fri, 12 Aug 2016 13:44:19 -0700 | Subject | Re: [PATCH v3 50/51] x86/mm: move arch_within_stack_frames() to usercopy.c |
| |
On Fri, Aug 12, 2016 at 1:36 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > On Fri, Aug 12, 2016 at 01:06:41PM -0700, Kees Cook wrote: >> On Fri, Aug 12, 2016 at 12:12 PM, Josh Poimboeuf <jpoimboe@redhat.com> 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@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"). >> > >> > Ok, I'll do that with the next patch (51/51) which removes the >> > __builtin_frame_address() calls. >> > >> >> > 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. >> > >> > 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 > > Ah. Do you expect other uses for it?
None that I'm aware of. :)
-Kees
> >> > 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. > > Oh, right. > > -- > Josh
-- Kees Cook Nexus Security
| |