| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 12 Aug 2016 10:36:21 -0700
From: Kees Cook <keescook@...omium.org>
To: Josh Poimboeuf <jpoimboe@...hat.com>
Cc: Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...nel.org>,
"H . Peter Anvin" <hpa@...or.com>,
"x86@...nel.org" <x86@...nel.org>,
LKML <linux-kernel@...r.kernel.org>,
Andy Lutomirski <luto@...capital.net>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Steven Rostedt <rostedt@...dmis.org>,
Brian Gerst <brgerst@...il.com>,
Peter Zijlstra <peterz@...radead.org>,
Frederic Weisbecker <fweisbec@...il.com>,
Byungchul Park <byungchul.park@....com>,
Nilay Vaish <nilayvaish@...il.com>
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@...hat.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@...hat.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
Powered by blists - more mailing lists