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

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.