Re: [kernel-hardening] Re: [PATCH 9/9] mm: SLUB hardened usercopy support

On Thu, Jul 7, 2016 at 12:35 AM, Michael Ellerman [off-list ref] wrote:

Kees Cook [off-list ref] writes:

quoted

Under CONFIG_HARDENED_USERCOPY, this adds object size checking to the
SLUB allocator to catch any copies that may span objects.

Based on code from PaX and grsecurity.

Signed-off-by: Kees Cook <redacted>

quoted

diff --git a/mm/slub.c b/mm/slub.c
index 825ff4505336..0c8ace04f075 100644
--- a/mm/slub.c
+++ b/mm/slub.c

@@ -3614,6 +3614,33 @@ void *__kmalloc_node(size_t size, gfp_t flags, int node)
 EXPORT_SYMBOL(__kmalloc_node);
 #endif

+#ifdef CONFIG_HARDENED_USERCOPY
+/*
+ * Rejects objects that are incorrectly sized.
+ *
+ * Returns NULL if check passes, otherwise const char * to name of cache
+ * to indicate an error.
+ */
+const char *__check_heap_object(const void *ptr, unsigned long n,
+                             struct page *page)
+{
+     struct kmem_cache *s;
+     unsigned long offset;
+
+     /* Find object. */
+     s = page->slab_cache;
+
+     /* Find offset within object. */
+     offset = (ptr - page_address(page)) % s->size;
+
+     /* Allow address range falling entirely within object size. */
+     if (offset <= s->object_size && n <= s->object_size - offset)
+             return NULL;
+
+     return s->name;
+}

I gave this a quick spin on powerpc, it blew up immediately :)

Wheee :) This series is rather easy to test: blows up REALLY quickly
if it's wrong. ;)

FWIW, -next also has a bunch of additional lkdtm tests for the various
protections and directions.

Ah, interesting. Just to make sure: you've built with
CONFIG_SLUB_DEBUG and either CONFIG_SLUB_DEBUG_ON or booted with
either slub_debug or slub_debug=z ?

Thanks for the slub fix!

I wonder if this code should be using size_from_object() instead of s->size?

(It looks like slab is already handling this via the obj_offset() call.)

-Kees

        /* Find offset within object. */
        offset = (ptr - page_address(page)) % s->size;

cheers

-- 
Kees Cook
Chrome OS & Brillo Security

Kees Cook [off-list ref] writes:

On Thu, Jul 7, 2016 at 12:35 AM, Michael Ellerman [off-list ref] wrote:

quoted

I gave this a quick spin on powerpc, it blew up immediately :)

Wheee :) This series is rather easy to test: blows up REALLY quickly
if it's wrong. ;)

Better than subtle race conditions which is the usual :)

quoted

diff --git a/mm/slub.c b/mm/slub.c
index 0c8ace04f075..66191ea4545a 100644
--- a/mm/slub.c
+++ b/mm/slub.c

@@ -3630,6 +3630,9 @@ const char *__check_heap_object(const void *ptr, unsigned long n,
        /* Find object. */
        s = page->slab_cache;

+       /* Subtract red zone if enabled */
+       ptr = restore_red_left(s, ptr);
+

Ah, interesting. Just to make sure: you've built with
CONFIG_SLUB_DEBUG and either CONFIG_SLUB_DEBUG_ON or booted with
either slub_debug or slub_debug=z ?

Yeah built with CONFIG_SLUB_DEBUG_ON, and booted with and without slub_debug
options.

Thanks for the slub fix!

I wonder if this code should be using size_from_object() instead of s->size?

Hmm, not sure. Who's SLUB maintainer? :)

I was modelling it on the logic in check_valid_pointer(), which also does the
restore_red_left(), and then checks for % s->size:

static inline int check_valid_pointer(struct kmem_cache *s,
				struct page *page, void *object)
{
	void *base;

	if (!object)
		return 1;

	base = page_address(page);
	object = restore_red_left(s, object);
	if (object < base || object >= base + page->objects * s->size ||
		(object - base) % s->size) {
		return 0;
	}

	return 1;
}

cheers

On Fri, 8 Jul 2016, Michael Ellerman wrote:

quoted

I wonder if this code should be using size_from_object() instead of s->size?

Hmm, not sure. Who's SLUB maintainer? :)

Me.

s->size is the size of the whole object including debugging info etc.
ksize() gives you the actual usable size of an object.

On Fri, Jul 8, 2016 at 9:45 AM, Christoph Lameter [off-list ref] wrote:

On Fri, 8 Jul 2016, Michael Ellerman wrote:

quoted

quoted

I wonder if this code should be using size_from_object() instead of s->size?

BTW, I can't reproduce this on x86 yet...

quoted

Hmm, not sure. Who's SLUB maintainer? :)

Me.

s->size is the size of the whole object including debugging info etc.
ksize() gives you the actual usable size of an object.

Is check_valid_pointer() making sure the pointer is within the usable
size? It seemed like it was checking that it was within the slub
object (checks against s->size, wants it above base after moving
pointer to include redzone, etc).

I think a potential problem with Michael's fix is that the ptr in
__check_heap_object() may not point at the _start_ of the usable
object, so doing the red zone shift isn't quite right.

This finds the ptr's offset within the slub object (since s->size is
the slub object size):

        offset = (ptr - page_address(page)) % s->size;

But this looks at object_size and doesn't take into account actual size:

        if (offset <= s->object_size && n <= s->object_size - offset)
                return NULL;

I think offset needs to be adjusted by the size of padding, which the
restore_red_left() call had the same effect, but may not cover all
padding conditions? I'm not sure.

Should it be:

        /* Find offset within slab object. */
        offset = (ptr - page_address(page)) % s->size;

        /* Adjust offset for meta data and padding. */
        offset -= s->size - s->object_size;

        /* Make sure offset and size are within bounds of the
allocation size. */
        if (offset <= s->object_size && n <= s->object_size - offset)
                return NULL;

?

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

On Fri, 8 Jul 2016, Kees Cook wrote:

Is check_valid_pointer() making sure the pointer is within the usable
size? It seemed like it was checking that it was within the slub
object (checks against s->size, wants it above base after moving
pointer to include redzone, etc).

check_valid_pointer verifies that a pointer is pointing to the start of an
object. It is used to verify the internal points that SLUB used and
should not be modified to do anything different.

On Fri, Jul 8, 2016 at 12:20 PM, Christoph Lameter [off-list ref] wrote:

On Fri, 8 Jul 2016, Kees Cook wrote:

quoted

Is check_valid_pointer() making sure the pointer is within the usable
size? It seemed like it was checking that it was within the slub
object (checks against s->size, wants it above base after moving
pointer to include redzone, etc).

check_valid_pointer verifies that a pointer is pointing to the start of an
object. It is used to verify the internal points that SLUB used and
should not be modified to do anything different.

Yup, no worries -- I won't touch it. :) I just wanted to verify my
understanding.

And after playing a bit more, I see that the only thing to the left is
padding and redzone. SLUB layout, from what I saw:

offset: what's there
-------
start: padding, redzone
red_left_pad: object itself
inuse: rest of metadata
size: start of next slub object

(and object_size == inuse - red_left_pad)

i.e. a pointer must be between red_left_pad and inuse, which is the
same as pointer - ref_left_pad being less than object_size.

So, as found already, the position in the usercopy check needs to be
bumped down by red_left_pad, which is what Michael's fix does, so I'll
include it in the next version.

Thanks!

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

On Fri, Jul 8, 2016 at 1:41 PM, Kees Cook [off-list ref] wrote:

On Fri, Jul 8, 2016 at 12:20 PM, Christoph Lameter [off-list ref] wrote:

quoted

On Fri, 8 Jul 2016, Kees Cook wrote:

quoted

Is check_valid_pointer() making sure the pointer is within the usable
size? It seemed like it was checking that it was within the slub
object (checks against s->size, wants it above base after moving
pointer to include redzone, etc).

check_valid_pointer verifies that a pointer is pointing to the start of an
object. It is used to verify the internal points that SLUB used and
should not be modified to do anything different.

Yup, no worries -- I won't touch it. :) I just wanted to verify my
understanding.

And after playing a bit more, I see that the only thing to the left is
padding and redzone. SLUB layout, from what I saw:

offset: what's there
-------
start: padding, redzone
red_left_pad: object itself
inuse: rest of metadata
size: start of next slub object

(and object_size == inuse - red_left_pad)

i.e. a pointer must be between red_left_pad and inuse, which is the
same as pointer - ref_left_pad being less than object_size.

So, as found already, the position in the usercopy check needs to be
bumped down by red_left_pad, which is what Michael's fix does, so I'll
include it in the next version.

Actually, after some offline chats, I think this is better, since it
makes sure the ptr doesn't end up somewhere weird before we start the
calculations. This leaves the pointer as-is, but explicitly handles
the redzone on the offset instead, with no wrapping, etc:

        /* Find offset within object. */
        offset = (ptr - page_address(page)) % s->size;

+       /* Adjust for redzone and reject if within the redzone. */
+       if (s->flags & SLAB_RED_ZONE) {
+               if (offset < s->red_left_pad)
+                       return s->name;
+               offset -= s->red_left_pad;
+       }
+
        /* Allow address range falling entirely within object size. */
        if (offset <= s->object_size && n <= s->object_size - offset)
                return NULL;

-Kees


-- 
Kees Cook
Chrome OS & Brillo Security

Kees Cook [off-list ref] writes:

On Fri, Jul 8, 2016 at 1:41 PM, Kees Cook [off-list ref] wrote:

quoted

So, as found already, the position in the usercopy check needs to be
bumped down by red_left_pad, which is what Michael's fix does, so I'll
include it in the next version.

Actually, after some offline chats, I think this is better, since it
makes sure the ptr doesn't end up somewhere weird before we start the
calculations. This leaves the pointer as-is, but explicitly handles
the redzone on the offset instead, with no wrapping, etc:

        /* Find offset within object. */
        offset = (ptr - page_address(page)) % s->size;

+       /* Adjust for redzone and reject if within the redzone. */
+       if (s->flags & SLAB_RED_ZONE) {
+               if (offset < s->red_left_pad)
+                       return s->name;
+               offset -= s->red_left_pad;
+       }
+
        /* Allow address range falling entirely within object size. */
        if (offset <= s->object_size && n <= s->object_size - offset)
                return NULL;

That fixes the case for me in kstrndup(), which allows the system to boot.

I then get two hits, which may or may not be valid:

[    2.309556] usercopy: kernel memory overwrite attempt detected to d000000003510028 (kernfs_node_cache) (64 bytes)
[    2.309995] CPU: 7 PID: 2241 Comm: wait-for-root Not tainted 4.7.0-rc3-00099-g97872fc89d41 #64
[    2.310480] Call Trace:
[    2.310556] [c0000001f4773bf0] [c0000000009bdbe8] dump_stack+0xb0/0xf0 (unreliable)
[    2.311016] [c0000001f4773c30] [c00000000029cf44] __check_object_size+0x74/0x320
[    2.311472] [c0000001f4773cb0] [c00000000005d4d0] copy_from_user+0x60/0xd4
[    2.311873] [c0000001f4773cf0] [c0000000008b38f4] __get_filter+0x74/0x160
[    2.312230] [c0000001f4773d30] [c0000000008b408c] sk_attach_filter+0x2c/0xc0
[    2.312596] [c0000001f4773d60] [c000000000871c34] sock_setsockopt+0x954/0xc00
[    2.313021] [c0000001f4773dd0] [c00000000086ac44] SyS_setsockopt+0x134/0x150
[    2.313380] [c0000001f4773e30] [c000000000009260] system_call+0x38/0x108
[    2.317045] usercopy: kernel memory overwrite attempt detected to d000000003530028 (kernfs_node_cache) (64 bytes)
[    2.317297] CPU: 10 PID: 2242 Comm: wait-for-root Not tainted 4.7.0-rc3-00099-g97872fc89d41 #64
[    2.317475] Call Trace:
[    2.317511] [c0000001f471fbf0] [c0000000009bdbe8] dump_stack+0xb0/0xf0 (unreliable)
[    2.317689] [c0000001f471fc30] [c00000000029cf44] __check_object_size+0x74/0x320
[    2.317861] [c0000001f471fcb0] [c00000000005d4d0] copy_from_user+0x60/0xd4
[    2.318011] [c0000001f471fcf0] [c0000000008b38f4] __get_filter+0x74/0x160
[    2.318165] [c0000001f471fd30] [c0000000008b408c] sk_attach_filter+0x2c/0xc0
[    2.318313] [c0000001f471fd60] [c000000000871c34] sock_setsockopt+0x954/0xc00
[    2.318485] [c0000001f471fdd0] [c00000000086ac44] SyS_setsockopt+0x134/0x150
[    2.318632] [c0000001f471fe30] [c000000000009260] system_call+0x38/0x108


With:

# zgrep SLUB /proc/config.gz
CONFIG_SLUB_DEBUG=y
CONFIG_SLUB=y
CONFIG_SLUB_CPU_PARTIAL=y
CONFIG_SLUB_DEBUG_ON=y
# CONFIG_SLUB_STATS is not set

cheers

Michael Ellerman [off-list ref] writes:

Kees Cook [off-list ref] writes:

quoted

On Fri, Jul 8, 2016 at 1:41 PM, Kees Cook [off-list ref] wrote:

quoted

So, as found already, the position in the usercopy check needs to be
bumped down by red_left_pad, which is what Michael's fix does, so I'll
include it in the next version.

Actually, after some offline chats, I think this is better, since it
makes sure the ptr doesn't end up somewhere weird before we start the
calculations. This leaves the pointer as-is, but explicitly handles
the redzone on the offset instead, with no wrapping, etc:

        /* Find offset within object. */
        offset = (ptr - page_address(page)) % s->size;

+       /* Adjust for redzone and reject if within the redzone. */
+       if (s->flags & SLAB_RED_ZONE) {
+               if (offset < s->red_left_pad)
+                       return s->name;
+               offset -= s->red_left_pad;
+       }
+
        /* Allow address range falling entirely within object size. */
        if (offset <= s->object_size && n <= s->object_size - offset)
                return NULL;

That fixes the case for me in kstrndup(), which allows the system to boot.

Ugh, no it doesn't, booted the wrong kernel.

I don't see the oops in strndup_user(), but instead get:

usercopy: kernel memory overwrite attempt detected to d000000003610028 (cfq_io_cq) (88 bytes)
CPU: 11 PID: 1 Comm: systemd Not tainted 4.7.0-rc3-00098-g09d9556ae5d1-dirty #65
Call Trace:
[c0000001fb087bf0] [c0000000009bdbe8] dump_stack+0xb0/0xf0 (unreliable)
[c0000001fb087c30] [c00000000029cf44] __check_object_size+0x74/0x320
[c0000001fb087cb0] [c00000000005d4d0] copy_from_user+0x60/0xd4
[c0000001fb087cf0] [c0000000008b38f4] __get_filter+0x74/0x160
[c0000001fb087d30] [c0000000008b408c] sk_attach_filter+0x2c/0xc0
[c0000001fb087d60] [c000000000871c34] sock_setsockopt+0x954/0xc00
[c0000001fb087dd0] [c00000000086ac44] SyS_setsockopt+0x134/0x150
[c0000001fb087e30] [c000000000009260] system_call+0x38/0x108
Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009

cheers

On Fri, Jul 08, 2016 at 04:48:38PM -0400, Kees Cook wrote:

On Fri, Jul 8, 2016 at 1:41 PM, Kees Cook [off-list ref] wrote:

quoted

On Fri, Jul 8, 2016 at 12:20 PM, Christoph Lameter [off-list ref] wrote:

quoted

On Fri, 8 Jul 2016, Kees Cook wrote:

quoted

Is check_valid_pointer() making sure the pointer is within the usable
size? It seemed like it was checking that it was within the slub
object (checks against s->size, wants it above base after moving
pointer to include redzone, etc).

check_valid_pointer verifies that a pointer is pointing to the start of an
object. It is used to verify the internal points that SLUB used and
should not be modified to do anything different.

Yup, no worries -- I won't touch it. :) I just wanted to verify my
understanding.

And after playing a bit more, I see that the only thing to the left is
padding and redzone. SLUB layout, from what I saw:

offset: what's there
-------
start: padding, redzone
red_left_pad: object itself
inuse: rest of metadata
size: start of next slub object

(and object_size == inuse - red_left_pad)

i.e. a pointer must be between red_left_pad and inuse, which is the
same as pointer - ref_left_pad being less than object_size.

So, as found already, the position in the usercopy check needs to be
bumped down by red_left_pad, which is what Michael's fix does, so I'll
include it in the next version.

Actually, after some offline chats, I think this is better, since it
makes sure the ptr doesn't end up somewhere weird before we start the
calculations. This leaves the pointer as-is, but explicitly handles
the redzone on the offset instead, with no wrapping, etc:

        /* Find offset within object. */
        offset = (ptr - page_address(page)) % s->size;

+       /* Adjust for redzone and reject if within the redzone. */
+       if (s->flags & SLAB_RED_ZONE) {
+               if (offset < s->red_left_pad)
+                       return s->name;
+               offset -= s->red_left_pad;
+       }
+
        /* Allow address range falling entirely within object size. */
        if (offset <= s->object_size && n <= s->object_size - offset)
                return NULL;

As Christoph saids, please use slab_ksize() rather than
s->object_size.

Otherwise, looks good to me.

Thanks.