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

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

[Christoph Lameter]

On Fri, 8 Jul 2016, Michael Ellerman wrote:

> > 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.

--
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>

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

[Kees Cook]

On Fri, Jul 8, 2016 at 9:45 AM, Christoph Lameter <cl@linux.com> wrote:
> On Fri, 8 Jul 2016, Michael Ellerman wrote:
>
>> > I wonder if this code should be using size_from_object() instead of s->size?

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

>>
>> 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

--
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>

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

[Christoph Lameter]

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.

--
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>

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

[Kees Cook]

On Fri, Jul 8, 2016 at 12:20 PM, Christoph Lameter <cl@linux.com> wrote:
> 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.

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

--
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>

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

[Kees Cook]

On Fri, Jul 8, 2016 at 1:41 PM, Kees Cook <keescook@chromium.org> wrote:
> On Fri, Jul 8, 2016 at 12:20 PM, Christoph Lameter <cl@linux.com> wrote:
>> 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.
>
> 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

--
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>

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

[Michael Ellerman]

Kees Cook <keescook@chromium.org> writes:

> On Fri, Jul 8, 2016 at 1:41 PM, Kees Cook <keescook@chromium.org> wrote:
>> 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

--
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>

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

[Michael Ellerman]

Kees Cook <keescook@chromium.org> writes:

> On Fri, Jul 8, 2016 at 1:41 PM, Kees Cook <keescook@chromium.org> wrote:
>> 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

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

[Michael Ellerman]

Kees Cook <keescook@chromium.org> writes:

> On Fri, Jul 8, 2016 at 1:41 PM, Kees Cook <keescook@chromium.org> wrote:
>> 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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

[Michael Ellerman]

Kees Cook <keescook@chromium.org> writes:

> On Fri, Jul 8, 2016 at 1:41 PM, Kees Cook <keescook@chromium.org> wrote:
>> 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

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

[Michael Ellerman]

Kees Cook <keescook@chromium.org> writes:

> On Fri, Jul 8, 2016 at 1:41 PM, Kees Cook <keescook@chromium.org> wrote:
>> 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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

[Michael Ellerman]

Michael Ellerman <mpe@ellerman.id.au> writes:

> Kees Cook <keescook@chromium.org> writes:
>
>> On Fri, Jul 8, 2016 at 1:41 PM, Kees Cook <keescook@chromium.org> wrote:
>>> 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

--
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>

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

[Michael Ellerman]

Michael Ellerman <mpe@ellerman.id.au> writes:

> Kees Cook <keescook@chromium.org> writes:
>
>> On Fri, Jul 8, 2016 at 1:41 PM, Kees Cook <keescook@chromium.org> wrote:
>>> 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

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

[Michael Ellerman]

Michael Ellerman <mpe@ellerman.id.au> writes:

> Kees Cook <keescook@chromium.org> writes:
>
>> On Fri, Jul 8, 2016 at 1:41 PM, Kees Cook <keescook@chromium.org> wrote:
>>> 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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

[Michael Ellerman]

Michael Ellerman <mpe@ellerman.id.au> writes:

> Kees Cook <keescook@chromium.org> writes:
>
>> On Fri, Jul 8, 2016 at 1:41 PM, Kees Cook <keescook@chromium.org> wrote:
>>> 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

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

[Michael Ellerman]

Michael Ellerman <mpe@ellerman.id.au> writes:

> Kees Cook <keescook@chromium.org> writes:
>
>> On Fri, Jul 8, 2016 at 1:41 PM, Kees Cook <keescook@chromium.org> wrote:
>>> 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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 2070 bytes --]

On Sat, 09 Jul 2016 15:58:20 +1000, Michael Ellerman said:

> 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

Yeah, 'ping' dies with a similar traceback going to rawv6_setsockopt(),
and 'trinity' dies a horrid death during initialization because it creates
some sctp sockets to fool around with.  The problem in all these cases is that
setsockopt uses copy_from_user() to pull in the option value, and the allocation
isn't tagged with USERCOPY to whitelist it.

Unfortunately, I haven't been able to track down where in net/ the memory is
allocated, nor is there any good hint in the grsecurity patch that I can find
where they do the tagging.

And the fact that so far, I'm only had ping and trinity killed in setsockopt()
hints that *most* setsockopt() calls must be going through a code path that
does allocate suitable memory, and these two have different paths.  I can't
believe they're the only two binaries that call setsockopt().....

Just saw your second mail, now I'm wondering why *my* laptop doesn't die a
horrid death when systemd starts up.  Mine is
systemd-230-3.gitea68351.fc25.x86_64 - maybe there's something
release-dependent going on?


[-- Attachment #2: Type: application/pgp-signature, Size: 848 bytes --]

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

[Kees Cook]

On Fri, Jul 8, 2016 at 11:17 PM,  <Valdis.Kletnieks@vt.edu> wrote:
> Yeah, 'ping' dies with a similar traceback going to rawv6_setsockopt(),
> and 'trinity' dies a horrid death during initialization because it creates
> some sctp sockets to fool around with.  The problem in all these cases is that
> setsockopt uses copy_from_user() to pull in the option value, and the allocation
> isn't tagged with USERCOPY to whitelist it.

Just a note to clear up confusion: this series doesn't include the
whitelist protection, so this appears to be either bugs in the slub
checker or bugs in the code using the cfq_io_cq cache. I suspect the
former. :)

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

--
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>

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

[Joonsoo Kim]

On Fri, Jul 08, 2016 at 04:48:38PM -0400, Kees Cook wrote:
> On Fri, Jul 8, 2016 at 1:41 PM, Kees Cook <keescook@chromium.org> wrote:
> > On Fri, Jul 8, 2016 at 12:20 PM, Christoph Lameter <cl@linux.com> wrote:
> >> 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.
> >
> > 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.

--
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>

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

[Michael Ellerman]

Kees Cook <keescook@chromium.org> writes:
> On Thu, Jul 7, 2016 at 12:35 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
>> 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 :)

>> 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

--
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>

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

[Michael Ellerman]

Kees Cook <keescook@chromium.org> writes:
> On Thu, Jul 7, 2016 at 12:35 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
>> 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 :)

>> 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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

[Michael Ellerman]

Kees Cook <keescook@chromium.org> writes:
> On Thu, Jul 7, 2016 at 12:35 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
>> 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 :)

>> 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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

[Kees Cook]

On Thu, Jul 7, 2016 at 12:35 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> Kees Cook <keescook@chromium.org> writes:
>
>> 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 <keescook@chromium.org>
>
>> 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.

>
>   Brought up 16 CPUs
>   usercopy: kernel memory overwrite attempt detected to c0000001fe023868 (kmalloc-16) (9 bytes)
>   CPU: 8 PID: 103 Comm: kdevtmpfs Not tainted 4.7.0-rc3-00098-g09d9556ae5d1 #55
>   Call Trace:
>   [c0000001fa0cfb40] [c0000000009bdbe8] dump_stack+0xb0/0xf0 (unreliable)
>   [c0000001fa0cfb80] [c00000000029cf44] __check_object_size+0x74/0x320
>   [c0000001fa0cfc00] [c00000000005d4d0] copy_from_user+0x60/0xd4
>   [c0000001fa0cfc40] [c00000000022b6cc] memdup_user+0x5c/0xf0
>   [c0000001fa0cfc80] [c00000000022b90c] strndup_user+0x7c/0x110
>   [c0000001fa0cfcc0] [c0000000002d6c28] SyS_mount+0x58/0x180
>   [c0000001fa0cfd10] [c0000000005ee908] devtmpfsd+0x98/0x210
>   [c0000001fa0cfd80] [c0000000000df810] kthread+0x110/0x130
>   [c0000001fa0cfe30] [c0000000000095e8] ret_from_kernel_thread+0x5c/0x74
>
> SLUB tracing says:
>
>   TRACE kmalloc-16 alloc 0xc0000001fe023868 inuse=186 fp=0x          (null)
>
> Which is not 16-byte aligned, which seems to be caused by the red zone?
> The following patch fixes it for me, but I don't know SLUB enough to say
> if it's always correct.
>
>
> 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 ?

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

--
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>