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

[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

[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

[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

[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

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

[Valdis.Kletnieks at vt.edu]

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?

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 848 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160709/c8bed600/attachment.sig>

[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

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