Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1439462 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [kernel-hardening] Re: [PATCH 9/9] mm: SLUB hardened usercopy  support Christoph Lameter <cl@linux.com> - 2016-07-08 15:50 +0200
    Re: [kernel-hardening] Re: [PATCH 9/9] mm: SLUB hardened usercopy support Kees Cook <keescook@chromium.org> - 2016-07-08 18:10 +0200
      Re: [kernel-hardening] Re: [PATCH 9/9] mm: SLUB hardened usercopy  support Christoph Lameter <cl@linux.com> - 2016-07-08 18:30 +0200
        Re: [kernel-hardening] Re: [PATCH 9/9] mm: SLUB hardened usercopy support Kees Cook <keescook@chromium.org> - 2016-07-08 19:50 +0200
          Re: [kernel-hardening] Re: [PATCH 9/9] mm: SLUB hardened usercopy support Kees Cook <keescook@chromium.org> - 2016-07-08 22:50 +0200
            Re: [kernel-hardening] Re: [PATCH 9/9] mm: SLUB hardened usercopy support Valdis.Kletnieks@vt.edu - 2016-07-09 08:20 +0200
              Re: [kernel-hardening] Re: [PATCH 9/9] mm: SLUB hardened usercopy support Kees Cook <keescook@chromium.org> - 2016-07-09 19:10 +0200
            Re: [kernel-hardening] Re: [PATCH 9/9] mm: SLUB hardened usercopy  support Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-07-11 08:10 +0200

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

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.

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1439637

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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?

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1440272

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web