diff mbox

[v3,2/3] arm64: usercopy: Implement stack frame object validation

Message ID 1486296850-16045-2-git-send-email-kpark3469@gmail.com
State New, archived
Headers show

Commit Message

kpark3469@gmail.com Feb. 5, 2017, 12:14 p.m. UTC
From: Sahara <keun-o.park@darkmatter.ae>

This implements arch_within_stack_frames() for arm64 that should
validate if a given object is contained by a kernel stack frame.

Signed-off-by: Sahara <keun-o.park@darkmatter.ae>
Reviewed-by: James Morse <james.morse@arm.com>
---
 arch/arm64/Kconfig                   |  1 +
 arch/arm64/include/asm/thread_info.h | 64 ++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

Comments

Kees Cook Feb. 6, 2017, 10:34 p.m. UTC | #1
On Sun, Feb 5, 2017 at 4:14 AM,  <kpark3469@gmail.com> wrote:
> From: Sahara <keun-o.park@darkmatter.ae>
>
> This implements arch_within_stack_frames() for arm64 that should
> validate if a given object is contained by a kernel stack frame.
>
> Signed-off-by: Sahara <keun-o.park@darkmatter.ae>
> Reviewed-by: James Morse <james.morse@arm.com>
> ---
>  arch/arm64/Kconfig                   |  1 +
>  arch/arm64/include/asm/thread_info.h | 64 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 65 insertions(+)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 1117421..8bf70b4 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -97,6 +97,7 @@ config ARM64
>         select HAVE_SYSCALL_TRACEPOINTS
>         select HAVE_KPROBES
>         select HAVE_KRETPROBES if HAVE_KPROBES
> +       select HAVE_ARCH_WITHIN_STACK_FRAMES
>         select IOMMU_DMA if IOMMU_SUPPORT
>         select IRQ_DOMAIN
>         select IRQ_FORCED_THREADING
> diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
> index 46c3b93..70baad3 100644
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
> @@ -68,7 +68,71 @@ struct thread_info {
>  #define thread_saved_fp(tsk)   \
>         ((unsigned long)(tsk->thread.cpu_context.fp))
>
> +#define get_stack_start(fp) (fp + 2 * sizeof(void *))

This define is only used once: might be better to just leave it
open-coded below.

> +
> +/*
> + * Walks up the stack frames to make sure that the specified object is
> + * entirely contained by a single stack frame.
> + *
> + * Returns:
> + *     GOOD_FRAME      if within a frame
> + *     BAD_STACK       if placed across a frame boundary (or outside stack)
> + *     NOT_STACK       unable to determine (no frame pointers, etc)
> + */
> +
> +static inline enum stack_type arch_within_stack_frames(const void * const stack,
> +                                       const void * const stackend,
> +                                       const void *obj, unsigned long len)
> +{
> +#ifdef CONFIG_FRAME_POINTER
> +       const void *oldframe = NULL;
> +       const void *frame = NULL;

I've renamed your variables back to match the x86 implementation so I
can more easily compare the differences. Since oldframe is always
initialized, it doesn't need the = NULL above.

> +
> +       oldframe = __builtin_frame_address(1);
> +       if (oldframe)
> +               frame = *(const void * const *)oldframe;

Why this instead of __builting_frame_address(2)? Just to avoid the "2" argument?

> +       /*
> +        * Case #1:
> +        * low ----------------------------------------------> high
> +        * [callee_fp][lr][args][local vars][caller_fp'][lr']
> +        *                ^----------------^
> +        *               allow copies only within here
> +        *
> +        * Case #2:
> +        * low ----------------------------------------------> high
> +        * [check_object_size_fp][lr][args][local vars][callee_fp][lr]
> +        *                           ^----------------^
> +        *                     dynamically allocated stack variable of
> +        *                     callee frame copies are allowed within here

I don't understand how these are different cases. We're walking the
entire stack, so each frame comparison should be the same.

> +        *
> +        * < example code snippet for Case#2 >
> +        * array_size = get_random_int() & 0x0f;
> +        * if (to_user) {
> +        *         unsigned char array[array_size];
> +        *         if (copy_to_user((void __user *)user_addr, array,
> +        *                          unconst + sizeof(array))) {
> +        */
> +       while (stack <= oldframe && oldframe < stackend &&

Why does this examine oldframe instead of the current frame?

> +                       !((unsigned long)frame & 0xf)) {

What is the 0xf test?

> +               /*
> +                * If obj + len extends past the caller 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 (!frame || (obj + len <= frame))

Excepting the initial frame test, this is identical to x86, which seems fine.

> +                       return (obj >= oldframe + 2 * sizeof(void *) ?
> +                               GOOD_FRAME : BAD_STACK;
> +               oldframe = frame;
> +               frame = *(const void * const *)frame;
> +       }
> +       return BAD_STACK;
> +#else
> +       return NOT_STACK;
>  #endif
> +}
> +
> +#endif /* !__ASSEMBLY__ */
>
>  /*
>   * thread information flags:
> --
> 2.7.4
>

I guess I just don't see the special case for arm64 frames?

-Kees
James Morse Feb. 7, 2017, 10:19 a.m. UTC | #2
On 05/02/17 12:14, kpark3469@gmail.com wrote:
> From: Sahara <keun-o.park@darkmatter.ae>
> 
> This implements arch_within_stack_frames() for arm64 that should
> validate if a given object is contained by a kernel stack frame.
> 
> Signed-off-by: Sahara <keun-o.park@darkmatter.ae>

> Reviewed-by: James Morse <james.morse@arm.com>

Careful, you should only include tags like this when they are explicitly given.
I don't remember doing that, and don't see it here:
http://www.openwall.com/lists/kernel-hardening/2017/01/26/8

I'd like to avoid having two sets of code that walk the stack.
I will have a go at a version of this patch that uses arm64s existing
walk_stackframe() machinery - lets find out if there is a good reason not to do
it that way!


Thanks,

James
diff mbox

Patch

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 1117421..8bf70b4 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -97,6 +97,7 @@  config ARM64
 	select HAVE_SYSCALL_TRACEPOINTS
 	select HAVE_KPROBES
 	select HAVE_KRETPROBES if HAVE_KPROBES
+	select HAVE_ARCH_WITHIN_STACK_FRAMES
 	select IOMMU_DMA if IOMMU_SUPPORT
 	select IRQ_DOMAIN
 	select IRQ_FORCED_THREADING
diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
index 46c3b93..70baad3 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -68,7 +68,71 @@  struct thread_info {
 #define thread_saved_fp(tsk)	\
 	((unsigned long)(tsk->thread.cpu_context.fp))
 
+#define get_stack_start(fp) (fp + 2 * sizeof(void *))
+
+/*
+ * Walks up the stack frames to make sure that the specified object is
+ * entirely contained by a single stack frame.
+ *
+ * Returns:
+ *	GOOD_FRAME	if within a frame
+ *	BAD_STACK	if placed across a frame boundary (or outside stack)
+ *	NOT_STACK	unable to determine (no frame pointers, etc)
+ */
+
+static inline enum stack_type arch_within_stack_frames(const void * const stack,
+					const void * const stackend,
+					const void *obj, unsigned long len)
+{
+#ifdef CONFIG_FRAME_POINTER
+	const void *callee_fp = NULL;
+	const void *caller_fp = NULL;
+
+	callee_fp = __builtin_frame_address(1);
+	if (callee_fp)
+		caller_fp = *(const void * const *)callee_fp;
+	/*
+	 * Case #1:
+	 * low ----------------------------------------------> high
+	 * [callee_fp][lr][args][local vars][caller_fp'][lr']
+	 *                ^----------------^
+	 *               allow copies only within here
+	 *
+	 * Case #2:
+	 * low ----------------------------------------------> high
+	 * [check_object_size_fp][lr][args][local vars][callee_fp][lr]
+	 *                           ^----------------^
+	 *                     dynamically allocated stack variable of
+	 *                     callee frame copies are allowed within here
+	 *
+	 * < example code snippet for Case#2 >
+	 * array_size = get_random_int() & 0x0f;
+	 * if (to_user) {
+	 *         unsigned char array[array_size];
+	 *         if (copy_to_user((void __user *)user_addr, array,
+	 *                          unconst + sizeof(array))) {
+	 */
+	while (stack <= callee_fp && callee_fp < stackend &&
+			!((unsigned long)caller_fp & 0xf)) {
+		/*
+		 * If obj + len extends past the caller 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 (!caller_fp || (obj + len <= caller_fp))
+			return (obj >= get_stack_start(callee_fp)) ?
+				GOOD_FRAME : BAD_STACK;
+		callee_fp = caller_fp;
+		caller_fp = *(const void * const *)caller_fp;
+	}
+	return BAD_STACK;
+#else
+	return NOT_STACK;
 #endif
+}
+
+#endif /* !__ASSEMBLY__ */
 
 /*
  * thread information flags: