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

Groups > linux.kernel > #1464643 > unrolled thread

Re: [x86/uaccess] 5b710f34e1: kernel BUG at mm/usercopy.c:75!

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: [x86/uaccess] 5b710f34e1: kernel BUG at mm/usercopy.c:75! Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-17 18:20 +0200
    Re: [x86/uaccess] 5b710f34e1: kernel BUG at mm/usercopy.c:75! Kees Cook <keescook@chromium.org> - 2016-08-17 23:30 +0200
      Re: [x86/uaccess] 5b710f34e1: kernel BUG at mm/usercopy.c:75! Kees Cook <keescook@chromium.org> - 2016-08-17 23:40 +0200
      Re: [x86/uaccess] 5b710f34e1: kernel BUG at mm/usercopy.c:75! Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-17 23:40 +0200
    Re: [x86/uaccess] 5b710f34e1: kernel BUG at mm/usercopy.c:75! Rik van Riel <riel@redhat.com> - 2016-08-17 23:50 +0200
      Re: [x86/uaccess] 5b710f34e1: kernel BUG at mm/usercopy.c:75! Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-18 00:00 +0200
        Re: [x86/uaccess] 5b710f34e1: kernel BUG at mm/usercopy.c:75! Kees Cook <keescook@chromium.org> - 2016-08-18 00:00 +0200
          Re: [x86/uaccess] 5b710f34e1: kernel BUG at mm/usercopy.c:75! Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-18 00:00 +0200
            Re: [x86/uaccess] 5b710f34e1: kernel BUG at mm/usercopy.c:75! Kees Cook <keescook@chromium.org> - 2016-08-18 00:00 +0200
      Re: [x86/uaccess] 5b710f34e1: kernel BUG at mm/usercopy.c:75! Kees Cook <keescook@chromium.org> - 2016-08-18 00:00 +0200
      Re: [x86/uaccess] 5b710f34e1: kernel BUG at mm/usercopy.c:75! Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-18 00:00 +0200
      Re: [x86/uaccess] 5b710f34e1: kernel BUG at mm/usercopy.c:75! Kees Cook <keescook@chromium.org> - 2016-08-18 00:00 +0200

#1464643 — Re: [x86/uaccess] 5b710f34e1: kernel BUG at mm/usercopy.c:75!

On Wed, Aug 17, 2016 at 5:17 AM, kernel test robot
<xiaolong.ye@intel.com> wrote:
>
> [  177.875629] usercopy: kernel memory overwrite attempt detected to 80028f40 (<spans multiple pages>) (512 bytes)

Ugh. This is a bug in the memory access hardening code.

I think it's this:

                        err = __copy_from_user(&fpu->state.xsave,
                                               buf_fx, state_size);

where it's copying the xsave area into the kernel buffer. That fpu
buffer is part of the thread structure:

                struct fpu *fpu = &tsk->thread.fpu;

and the thread struct allocation is two pages at 80028000:

> [  178.037761] task: 80028000 ti: 8002a000 task.ti: 8002a000

So yes, it "crosses" the page from 80028000 to 80029000, but the task
allocation is fine, at 80028000-8002a000.

The check_heap_object() code is simply buggy. It does seem to try to
handle this, by handling compound pages:

        /* Allow if start and end are inside the same compound page. */
        endpage = virt_to_head_page(end);
        if (likely(endpage == page))
                return NULL;

but compound pages are about the mapping of hugepages, not about
simple multi-order allocations like the task structure (or slab
entries).

In other words, it looks like the memory hardening is simply broken
for any case that doesn't use kmalloc(), but instead just allocates
non-order-0 pages directly. Which is certainly _rare_, but not unheard
of.

I'm not sure how to fix it.The low-level page allocator does *not*
mark orders anywhere.

I suspect we should just get rid of the page-crosser checking, because
it's unsolvable.

                  Linus

[toc] | [next] | [standalone]

#1464807

On Wed, Aug 17, 2016 at 9:14 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Aug 17, 2016 at 5:17 AM, kernel test robot
> <xiaolong.ye@intel.com> wrote:
>>
>> [  177.875629] usercopy: kernel memory overwrite attempt detected to 80028f40 (<spans multiple pages>) (512 bytes)
>
> Ugh. This is a bug in the memory access hardening code.
>
> I think it's this:
>
>                         err = __copy_from_user(&fpu->state.xsave,
>                                                buf_fx, state_size);
>
> where it's copying the xsave area into the kernel buffer. That fpu
> buffer is part of the thread structure:
>
>                 struct fpu *fpu = &tsk->thread.fpu;
>
> and the thread struct allocation is two pages at 80028000:
>
>> [  178.037761] task: 80028000 ti: 8002a000 task.ti: 8002a000
>
> So yes, it "crosses" the page from 80028000 to 80029000, but the task
> allocation is fine, at 80028000-8002a000.
>
> The check_heap_object() code is simply buggy. It does seem to try to
> handle this, by handling compound pages:
>
>         /* Allow if start and end are inside the same compound page. */
>         endpage = virt_to_head_page(end);
>         if (likely(endpage == page))
>                 return NULL;
>
> but compound pages are about the mapping of hugepages, not about
> simple multi-order allocations like the task structure (or slab
> entries).
>
> In other words, it looks like the memory hardening is simply broken
> for any case that doesn't use kmalloc(), but instead just allocates
> non-order-0 pages directly. Which is certainly _rare_, but not unheard
> of.
>
> I'm not sure how to fix it.The low-level page allocator does *not*
> mark orders anywhere.
>
> I suspect we should just get rid of the page-crosser checking, because
> it's unsolvable.

I had forwarded this bug Rik's way since the page-cross checking was
suggested by him. I'm happy to drop it; it was a suggested improvement
that was suspected to be safe (none of the folks testing this ran into
it and we saw no report during its time in -next). I can prepare a
patch if there isn't a better way to detect this kind of allocation.
(FWIW, slab is handled separately.)

-Kees

-- 
Kees Cook
Nexus Security

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

#1464815

On Wed, Aug 17, 2016 at 2:30 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Aug 17, 2016 at 2:25 PM, Kees Cook <keescook@chromium.org> wrote:
>>
>> I had forwarded this bug Rik's way since the page-cross checking was
>> suggested by him. I'm happy to drop it; it was a suggested improvement
>> that was suspected to be safe (none of the folks testing this ran into
>> it and we saw no report during its time in -next). I can prepare a
>> patch if there isn't a better way to detect this kind of allocation.
>> (FWIW, slab is handled separately.)
>
> I can't think of any sane way to notice it normally.
>
> Yes, with __GFP_COMPOUND you get the compound flag bits set, but as
> mentioned, that's a special case for the large page VM handling, and
> not applicable in general.
>
> Very few things do higher order allocations outside of slab and the
> task struct. But it does happen. Even fewer of those then have
> contents that might get copied to user space, but it clearly happens
> at least for _one_ case, and I can't convince myself that there might
> not be other cases too..

Yup, totally, I'll send a patch to remove this and Rik and I can
investigate re-adding it later.

-Kees

-- 
Kees Cook
Nexus Security

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

#1464818

On Wed, Aug 17, 2016 at 2:25 PM, Kees Cook <keescook@chromium.org> wrote:
>
> I had forwarded this bug Rik's way since the page-cross checking was
> suggested by him. I'm happy to drop it; it was a suggested improvement
> that was suspected to be safe (none of the folks testing this ran into
> it and we saw no report during its time in -next). I can prepare a
> patch if there isn't a better way to detect this kind of allocation.
> (FWIW, slab is handled separately.)

I can't think of any sane way to notice it normally.

Yes, with __GFP_COMPOUND you get the compound flag bits set, but as
mentioned, that's a special case for the large page VM handling, and
not applicable in general.

Very few things do higher order allocations outside of slab and the
task struct. But it does happen. Even fewer of those then have
contents that might get copied to user space, but it clearly happens
at least for _one_ case, and I can't convince myself that there might
not be other cases too..

                  Linus

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

#1464823

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

On Wed, 2016-08-17 at 09:14 -0700, Linus Torvalds wrote:

> but compound pages are about the mapping of hugepages, not about
> simple multi-order allocations like the task structure (or slab
> entries).
> 
> In other words, it looks like the memory hardening is simply broken
> for any case that doesn't use kmalloc(), but instead just allocates
> non-order-0 pages directly. Which is certainly _rare_, but not
> unheard
> of.
> 
> I'm not sure how to fix it.The low-level page allocator does *not*
> mark orders anywhere.
> 
> I suspect we should just get rid of the page-crosser checking,
> because
> it's unsolvable.

I glossed over the fact that prep_new_page only marks
the page as a compound page if __GFP_COMP is set, which
it is not for some higher order allocations, including
GFP masks passed straight through by the SLOB allocator.

This particular allocation is through kmalloc, but the
kernel in question has CONFIG_SLOB=y, and usercopy has
no code in mm/slob.c

I can think of two possibilities:
- mark every higher order allocation so it can be
  recognized later on (this might break allocators
  that free part of a higher order allocation, I
  do not know whether they exist)
- drop this part of the usercopy code, and no longer
  check that a range is part of an object that was
  allocated all at once, or spans multiple memory
  allocations - this may be ok, given that most
  heap overflows are likely to be kmalloc/slab objects,
  anyway

kind regards,

Rik
-- 

All Rights Reversed.

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

#1464829

On Wed, Aug 17, 2016 at 2:37 PM, Rik van Riel <riel@redhat.com> wrote:
>
> This particular allocation is through kmalloc, but the
> kernel in question has CONFIG_SLOB=y, and usercopy has
> no code in mm/slob.c

Oh, I didn't notice that.

Maybe we can just say that HARDENING depends on !SLOB for now, and see
if anything else shows up.

Maybe we don't have any code that copies data from (non-kmalloc)
multi-order allocations to user space.

Networking does, but seems to use __GFP_COMP, at least in the one case
I checked (skbuff).

           Linus

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

#1464831

On Wed, Aug 17, 2016 at 2:45 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Aug 17, 2016 at 2:37 PM, Rik van Riel <riel@redhat.com> wrote:
>>
>> This particular allocation is through kmalloc, but the
>> kernel in question has CONFIG_SLOB=y, and usercopy has
>> no code in mm/slob.c
>
> Oh, I didn't notice that.
>
> Maybe we can just say that HARDENING depends on !SLOB for now, and see
> if anything else shows up.

This logic (for avoiding uninstrumented allocators, which is only
SLOB) already exists (via CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR).
And PageSlab(page) should be catching this, so that the logic of "this
is from the allocator, so we must use its checker" is supposed to get
invoked.

> Maybe we don't have any code that copies data from (non-kmalloc)
> multi-order allocations to user space.
>
> Networking does, but seems to use __GFP_COMP, at least in the one case
> I checked (skbuff).

Was this allocation really through kmalloc?

-Kees

-- 
Kees Cook
Nexus Security

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

#1464834

On Wed, Aug 17, 2016 at 2:50 PM, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Aug 17, 2016 at 2:45 PM, Linus Torvalds
>>
>> Networking does, but seems to use __GFP_COMP, at least in the one case
>> I checked (skbuff).
>
> Was this allocation really through kmalloc?

The networking one I looked at, no. But they do __GFP_COMP.

The task struct allocation generally is (alloc_task_struct_node()),
but as Rik pointed out, SLOB doesn't actually necessarily do the slab
book-keeping for multi-page allocations.

               Linus

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

#1464835

On Wed, Aug 17, 2016 at 2:55 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Aug 17, 2016 at 2:50 PM, Kees Cook <keescook@chromium.org> wrote:
>> On Wed, Aug 17, 2016 at 2:45 PM, Linus Torvalds
>>>
>>> Networking does, but seems to use __GFP_COMP, at least in the one case
>>> I checked (skbuff).
>>
>> Was this allocation really through kmalloc?
>
> The networking one I looked at, no. But they do __GFP_COMP.
>
> The task struct allocation generally is (alloc_task_struct_node()),
> but as Rik pointed out, SLOB doesn't actually necessarily do the slab
> book-keeping for multi-page allocations.

Perhaps I can just ifdef the multi-page checks with
CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR. That way a SLOB build still
has basic bounds checking (which was my intention with that config),
and non-SLOB builds still get multi-page checking.

-Kees

-- 
Kees Cook
Nexus Security

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

#1464830

On Wed, Aug 17, 2016 at 2:52 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Aug 17, 2016 at 2:45 PM, Kees Cook <keescook@chromium.org> wrote:
>>
>> But PageSlab(page) should trip, returning __check_heap_object, which
>> for SLOB should just return NULL, skipping all the rest of the
>> checks...
>
> SLOB doesn't actually set that for all allocations.
>
> See "slob_alloc_node()", for example. It just returns a multi-order allocation.
>
> (See also kfree(), which uses PageSlab() to determine it it should do
> slob_free() or just free the pages directly).

Oooh, eww. Okay, that explains it. Alright, dropping all the
multi-page logic now...

-Kees

-- 
Kees Cook
Nexus Security

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

#1464832

On Wed, Aug 17, 2016 at 2:45 PM, Kees Cook <keescook@chromium.org> wrote:
>
> But PageSlab(page) should trip, returning __check_heap_object, which
> for SLOB should just return NULL, skipping all the rest of the
> checks...

SLOB doesn't actually set that for all allocations.

See "slob_alloc_node()", for example. It just returns a multi-order allocation.

(See also kfree(), which uses PageSlab() to determine it it should do
slob_free() or just free the pages directly).

         Linus

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

#1464833

On Wed, Aug 17, 2016 at 2:37 PM, Rik van Riel <riel@redhat.com> wrote:
> On Wed, 2016-08-17 at 09:14 -0700, Linus Torvalds wrote:
>
>> but compound pages are about the mapping of hugepages, not about
>> simple multi-order allocations like the task structure (or slab
>> entries).
>>
>> In other words, it looks like the memory hardening is simply broken
>> for any case that doesn't use kmalloc(), but instead just allocates
>> non-order-0 pages directly. Which is certainly _rare_, but not
>> unheard
>> of.
>>
>> I'm not sure how to fix it.The low-level page allocator does *not*
>> mark orders anywhere.
>>
>> I suspect we should just get rid of the page-crosser checking,
>> because
>> it's unsolvable.
>
> I glossed over the fact that prep_new_page only marks
> the page as a compound page if __GFP_COMP is set, which
> it is not for some higher order allocations, including
> GFP masks passed straight through by the SLOB allocator.
>
> This particular allocation is through kmalloc, but the
> kernel in question has CONFIG_SLOB=y, and usercopy has
> no code in mm/slob.c

But PageSlab(page) should trip, returning __check_heap_object, which
for SLOB should just return NULL, skipping all the rest of the
checks...

-Kees

>
> I can think of two possibilities:
> - mark every higher order allocation so it can be
>   recognized later on (this might break allocators
>   that free part of a higher order allocation, I
>   do not know whether they exist)
> - drop this part of the usercopy code, and no longer
>   check that a range is part of an object that was
>   allocated all at once, or spans multiple memory
>   allocations - this may be ok, given that most
>   heap overflows are likely to be kmalloc/slab objects,
>   anyway
>
> kind regards,
>
> Rik
> --
>
> All Rights Reversed.



-- 
Kees Cook
Nexus Security

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web