On Thu, Jul 21, 2016 at 11:34:25AM -0700, Kees Cook wrote:

On Wed, Jul 20, 2016 at 11:52 PM, Michael Ellerman [off-list ref] wrote:

quoted

Kees Cook [off-list ref] writes:

quoted

diff --git a/mm/usercopy.c b/mm/usercopy.c
new file mode 100644
index 000000000000..e4bf4e7ccdf6
--- /dev/null
+++ b/mm/usercopy.c

@@ -0,0 +1,234 @@

...

quoted

+
+/*
+ * Checks if a given pointer and length is contained by the current
+ * stack frame (if possible).
+ *
+ *   0: not at all on the stack
+ *   1: fully within a valid stack frame
+ *   2: fully on the stack (when can't do frame-checking)
+ *   -1: error condition (invalid stack position or bad stack frame)
+ */
+static noinline int check_stack_object(const void *obj, unsigned long len)
+{
+     const void * const stack = task_stack_page(current);
+     const void * const stackend = stack + THREAD_SIZE;

That allows access to the entire stack, including the struct thread_info,
is that what we want - it seems dangerous? Or did I miss a check
somewhere else?

That seems like a nice improvement to make, yeah.

quoted

We have end_of_stack() which computes the end of the stack taking
thread_info into account (end being the opposite of your end above).

Amusingly, the object_is_on_stack() check in sched.h doesn't take
thread_info into account either. :P Regardless, I think using
end_of_stack() may not be best. To tighten the check, I think we could
add this after checking that the object is on the stack:

#ifdef CONFIG_STACK_GROWSUP
        stackend -= sizeof(struct thread_info);
#else
        stack += sizeof(struct thread_info);
#endif

e.g. then if the pointer was in the thread_info, the second test would
fail, triggering the protection.

FWIW, this won't work right on x86 after Andy's
CONFIG_THREAD_INFO_IN_TASK patches get merged.

-- 
Josh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>