* [kernel-hardening] Re: [PATCH 9/9] mm: SLUB hardened usercopy support [not found] ` <alpine.DEB.2.20.1607081119170.6192@east.gentwo.org> @ 2016-07-08 17:41 ` Kees Cook 2016-07-08 20:48 ` Kees Cook 0 siblings, 1 reply; 7+ messages in thread From: Kees Cook @ 2016-07-08 17:41 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [kernel-hardening] Re: [PATCH 9/9] mm: SLUB hardened usercopy support 2016-07-08 17:41 ` [kernel-hardening] Re: [PATCH 9/9] mm: SLUB hardened usercopy support Kees Cook @ 2016-07-08 20:48 ` Kees Cook 2016-07-09 5:58 ` Michael Ellerman ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Kees Cook @ 2016-07-08 20:48 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [kernel-hardening] Re: [PATCH 9/9] mm: SLUB hardened usercopy support 2016-07-08 20:48 ` Kees Cook @ 2016-07-09 5:58 ` Michael Ellerman 2016-07-09 6:07 ` Michael Ellerman [not found] ` <57809299.84b3370a.5390c.ffff9e58SMTPIN_ADDED_BROKEN@mx.google.com> 2016-07-11 6:08 ` Joonsoo Kim 2 siblings, 1 reply; 7+ messages in thread From: Michael Ellerman @ 2016-07-09 5:58 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [kernel-hardening] Re: [PATCH 9/9] mm: SLUB hardened usercopy support 2016-07-09 5:58 ` Michael Ellerman @ 2016-07-09 6:07 ` Michael Ellerman 0 siblings, 0 replies; 7+ messages in thread From: Michael Ellerman @ 2016-07-09 6:07 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <57809299.84b3370a.5390c.ffff9e58SMTPIN_ADDED_BROKEN@mx.google.com>]
* [kernel-hardening] Re: [PATCH 9/9] mm: SLUB hardened usercopy support [not found] ` <57809299.84b3370a.5390c.ffff9e58SMTPIN_ADDED_BROKEN@mx.google.com> @ 2016-07-09 6:17 ` Valdis.Kletnieks at vt.edu 2016-07-09 17:07 ` Kees Cook 0 siblings, 1 reply; 7+ messages in thread From: Valdis.Kletnieks at vt.edu @ 2016-07-09 6:17 UTC (permalink / raw) To: linux-arm-kernel 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* [kernel-hardening] Re: [PATCH 9/9] mm: SLUB hardened usercopy support 2016-07-09 6:17 ` Valdis.Kletnieks at vt.edu @ 2016-07-09 17:07 ` Kees Cook 0 siblings, 0 replies; 7+ messages in thread From: Kees Cook @ 2016-07-09 17:07 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [kernel-hardening] Re: [PATCH 9/9] mm: SLUB hardened usercopy support 2016-07-08 20:48 ` Kees Cook 2016-07-09 5:58 ` Michael Ellerman [not found] ` <57809299.84b3370a.5390c.ffff9e58SMTPIN_ADDED_BROKEN@mx.google.com> @ 2016-07-11 6:08 ` Joonsoo Kim 2 siblings, 0 replies; 7+ messages in thread From: Joonsoo Kim @ 2016-07-11 6:08 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-07-11 6:08 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <577f7e55.4668420a.84f17.5cb9SMTPIN_ADDED_MISSING@mx.google.com> [not found] ` <alpine.DEB.2.20.1607080844370.3379@east.gentwo.org> [not found] ` <CAGXu5jKE=h32tHVLsDeaPN1GfC+BB3YbFvC+5TE5TK1oR-xU3A@mail.gmail.com> [not found] ` <alpine.DEB.2.20.1607081119170.6192@east.gentwo.org> 2016-07-08 17:41 ` [kernel-hardening] Re: [PATCH 9/9] mm: SLUB hardened usercopy support Kees Cook 2016-07-08 20:48 ` Kees Cook 2016-07-09 5:58 ` Michael Ellerman 2016-07-09 6:07 ` Michael Ellerman [not found] ` <57809299.84b3370a.5390c.ffff9e58SMTPIN_ADDED_BROKEN@mx.google.com> 2016-07-09 6:17 ` Valdis.Kletnieks at vt.edu 2016-07-09 17:07 ` Kees Cook 2016-07-11 6:08 ` Joonsoo Kim
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).