Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1461452
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [PATCH v3 50/51] x86/mm: move arch_within_stack_frames() to usercopy.c |
| Date | 2016-08-12 22:50 +0200 |
| Message-ID | <s5ryV-6EZ-3@gated-at.bofh.it> (permalink) |
| References | (1 earlier) <s5lMT-2Ra-61@gated-at.bofh.it> <s5oB4-4Ie-7@gated-at.bofh.it> <s5q9P-5PW-1@gated-at.bofh.it> <s5qWd-6qZ-5@gated-at.bofh.it> <s5rpf-6B6-13@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
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
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[PATCH v3 00/51] x86/dumpstack: rewrite x86 stack dump code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
[PATCH v3 49/51] x86/dumpstack: warn on stack recursion Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
[PATCH v3 36/51] perf/x86: convert perf_callchain_kernel() to use the new unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
[PATCH v3 26/51] ftrace: add ftrace_graph_ret_addr() stack unwinding helpers Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
[PATCH v3 33/51] x86/dumpstack: add get_stack_info() interface Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
[PATCH v3 48/51] x86/unwind: warn if stack grows up Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
Re: [PATCH v3 48/51] x86/unwind: warn if stack grows up Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:30 +0200
Re: [PATCH v3 48/51] x86/unwind: warn if stack grows up Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 18:30 +0200
[PATCH v3 50/51] x86/mm: move arch_within_stack_frames() to usercopy.c Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
Re: [PATCH v3 50/51] x86/mm: move arch_within_stack_frames() to usercopy.c Kees Cook <keescook@chromium.org> - 2016-08-12 19:40 +0200
Re: [PATCH v3 50/51] x86/mm: move arch_within_stack_frames() to usercopy.c Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 21:20 +0200
Re: [PATCH v3 50/51] x86/mm: move arch_within_stack_frames() to usercopy.c Kees Cook <keescook@chromium.org> - 2016-08-12 22:10 +0200
Re: [PATCH v3 50/51] x86/mm: move arch_within_stack_frames() to usercopy.c Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 22:40 +0200
Re: [PATCH v3 50/51] x86/mm: move arch_within_stack_frames() to usercopy.c Kees Cook <keescook@chromium.org> - 2016-08-12 22:50 +0200
[PATCH v3 44/51] x86/dumpstack: print any pt_regs found on the stack Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
Re: [PATCH v3 44/51] x86/dumpstack: print any pt_regs found on the stack Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:50 +0200
[PATCH v3 12/51] x86: move _stext marker to before head code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
[PATCH v3 32/51] x86/dumpstack: simplify in_exception_stack() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
Re: [PATCH v3 32/51] x86/dumpstack: simplify in_exception_stack() Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:50 +0200
Re: [PATCH v3 32/51] x86/dumpstack: simplify in_exception_stack() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 17:40 +0200
[PATCH v3 14/51] x86/asm/head: put real return address on idle task stack Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
Re: [PATCH v3 14/51] x86/asm/head: put real return address on idle task stack Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:40 +0200
Re: [PATCH v3 14/51] x86/asm/head: put real return address on idle task stack Nilay Vaish <nilayvaish@gmail.com> - 2016-08-17 22:40 +0200
Re: [PATCH v3 14/51] x86/asm/head: put real return address on idle task stack Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-17 23:20 +0200
[PATCH v3 16/51] x86/32: put real return address on stack in entry code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
Re: [PATCH v3 16/51] x86/32: put real return address on stack in entry code Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:30 +0200
Re: [PATCH v3 16/51] x86/32: put real return address on stack in entry code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 17:10 +0200
Re: [PATCH v3 16/51] x86/32: put real return address on stack in entry code "H. Peter Anvin" <hpa@zytor.com> - 2016-08-15 20:10 +0200
Re: [PATCH v3 16/51] x86/32: put real return address on stack in entry code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 20:30 +0200
Re: [PATCH v3 16/51] x86/32: put real return address on stack in entry code "H. Peter Anvin" <hpa@zytor.com> - 2016-08-15 21:30 +0200
Re: [PATCH v3 16/51] x86/32: put real return address on stack in entry code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 22:10 +0200
[PATCH v3 18/51] x86/entry/head/32: use local labels Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
[PATCH v3 30/51] x86/dumpstack/ftrace: don't print unreliable addresses in print_context_stack_bp() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
[PATCH v3 42/51] x86/unwind: create stack frames for saved syscall registers Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
Re: [PATCH v3 42/51] x86/unwind: create stack frames for saved syscall registers Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:40 +0200
Re: [PATCH v3 42/51] x86/unwind: create stack frames for saved syscall registers Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 19:00 +0200
[PATCH v3 28/51] ftrace/x86: implement HAVE_FUNCTION_GRAPH_RET_ADDR_PTR Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
[PATCH v3 31/51] x86/dumpstack: allow preemption in show_stack_log_lvl() and dump_trace() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
Re: [PATCH v3 31/51] x86/dumpstack: allow preemption in show_stack_log_lvl() and dump_trace() Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:50 +0200
Re: [PATCH v3 31/51] x86/dumpstack: allow preemption in show_stack_log_lvl() and dump_trace() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 17:40 +0200
[PATCH v3 41/51] x86/entry/unwind: create stack frames for saved interrupt registers Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
Re: [PATCH v3 41/51] x86/entry/unwind: create stack frames for saved interrupt registers Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:40 +0200
Re: [PATCH v3 41/51] x86/entry/unwind: create stack frames for saved interrupt registers Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 18:40 +0200
[PATCH v3 37/51] x86/stacktrace: convert save_stack_trace_*() to use the new unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
[PATCH v3 05/51] x86/dumpstack: make printk_stack_address() more generally useful Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
[PATCH v3 43/51] x86/dumpstack: print stack identifier on its own line Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
[PATCH v3 24/51] ftrace: only allocate the ret_stack 'fp' field when needed Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
[PATCH v3 29/51] x86/dumpstack/ftrace: mark function graph handler function as unreliable Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
[PATCH v3 03/51] x86/asm/head: rename 'stack_start' -> 'initial_stack' Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
[PATCH v3 09/51] x86/dumpstack: fix x86_32 kernel_stack_pointer() previous stack access Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
Re: [PATCH v3 09/51] x86/dumpstack: fix x86_32 kernel_stack_pointer() previous stack access Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:30 +0200
Re: [PATCH v3 09/51] x86/dumpstack: fix x86_32 kernel_stack_pointer() previous stack access Brian Gerst <brgerst@gmail.com> - 2016-08-14 15:00 +0200
Re: [PATCH v3 09/51] x86/dumpstack: fix x86_32 kernel_stack_pointer() previous stack access Andy Lutomirski <luto@amacapital.net> - 2016-08-14 15:50 +0200
Re: [PATCH v3 09/51] x86/dumpstack: fix x86_32 kernel_stack_pointer() previous stack access Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 17:10 +0200
Re: [PATCH v3 09/51] x86/dumpstack: fix x86_32 kernel_stack_pointer() previous stack access Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 19:30 +0200
Re: [PATCH v3 09/51] x86/dumpstack: fix x86_32 kernel_stack_pointer() previous stack access Andy Lutomirski <luto@amacapital.net> - 2016-08-15 22:10 +0200
[PATCH v3 13/51] x86/asm/head: remove useless zeroed word Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
[PATCH v3 21/51] oprofile/x86: add regs->ip to oprofile trace Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
[PATCH v3 10/51] x86/dumpstack: add get_stack_pointer() and get_frame_pointer() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
[PATCH v3 07/51] x86/dumpstack: remove extra brackets around "<EOE>" Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
[PATCH v3 17/51] x86/smp: fix initial idle stack location on 32-bit Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
[PATCH v3 08/51] x86/dumpstack: fix irq stack bounds calculation in show_stack_log_lvl() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
[PATCH v3 15/51] x86/asm/head: standardize the end of the stack for idle tasks Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
Re: [PATCH v3 15/51] x86/asm/head: standardize the end of the stack for idle tasks Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:40 +0200
[PATCH v3 19/51] x86/entry/32: rename 'error_code' to 'common_exception' Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
Re: [PATCH v3 19/51] x86/entry/32: rename 'error_code' to 'common_exception' Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:50 +0200
Re: [PATCH v3 19/51] x86/entry/32: rename 'error_code' to 'common_exception' Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 17:40 +0200
[PATCH v3 23/51] ftrace: remove CONFIG_HAVE_FUNCTION_GRAPH_FP_TEST from config Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
[PATCH v3 11/51] x86/dumpstack: remove unnecessary stack pointer arguments Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 17:10 +0200
csiph-web