[PATCH] usercopy: Skip multi-page bounds checking on SLOB

[PATCH] usercopy: Skip multi-page bounds checking on SLOB

[Kees Cook]

When an allocator does not mark all allocations as PageSlab, or does not
mark multipage allocations with __GFP_COMP, hardened usercopy cannot
correctly validate the allocation. SLOB lacks this, so short-circuit
the checking for the allocators that aren't marked with
CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR. This also updates the config
help and corrects a typo in the usercopy comments.

Reported-by: xiaolong.ye@intel.com
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 mm/usercopy.c    | 11 ++++++++++-
 security/Kconfig |  5 +++--
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/mm/usercopy.c b/mm/usercopy.c
index 8ebae91a6b55..855944b05cc7 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -172,6 +172,15 @@ static inline const char *check_heap_object(const void *ptr, unsigned long n,
 		return NULL;
 	}
 
+#ifndef CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR
+	/*
+	 * If the allocator isn't marking multi-page allocations as
+	 * either __GFP_COMP or PageSlab, we cannot correctly perform
+	 * bounds checking of multi-page allocations, so we stop here.
+	 */
+	return NULL;
+#endif
+
 	/* Allow kernel data region (if not marked as Reserved). */
 	if (ptr >= (const void *)_sdata && end <= (const void *)_edata)
 		return NULL;
@@ -192,7 +201,7 @@ static inline const char *check_heap_object(const void *ptr, unsigned long n,
 		return NULL;
 
 	/*
-	 * Reject if range is entirely either Reserved (i.e. special or
+	 * Allow if range is entirely either Reserved (i.e. special or
 	 * device memory), or CMA. Otherwise, reject since the object spans
 	 * several independently allocated pages.
 	 */
diff --git a/security/Kconfig b/security/Kconfig
index df28f2b6f3e1..08dce0327d5b 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -122,8 +122,9 @@ config HAVE_HARDENED_USERCOPY_ALLOCATOR
 	bool
 	help
 	  The heap allocator implements __check_heap_object() for
-	  validating memory ranges against heap object sizes in
-	  support of CONFIG_HARDENED_USERCOPY.
+	  validating memory ranges against heap object sizes in support
+	  of CONFIG_HARDENED_USERCOPY. It must mark all managed pages as
+	  PageSlab(), or set __GFP_COMP for multi-page allocations.
 
 config HAVE_ARCH_HARDENED_USERCOPY
 	bool
-- 
2.7.4


-- 
Kees Cook
Nexus Security

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] usercopy: Skip multi-page bounds checking on SLOB

[Rik van Riel]

On Wed, 2016-08-17 at 15:29 -0700, Kees Cook wrote:
> When an allocator does not mark all allocations as PageSlab, or does
> not
> mark multipage allocations with __GFP_COMP, hardened usercopy cannot
> correctly validate the allocation. SLOB lacks this, so short-circuit
> the checking for the allocators that aren't marked with
> CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR. This also updates the config
> help and corrects a typo in the usercopy comments.
> 
> Reported-by: xiaolong.ye@intel.com
> Signed-off-by: Kees Cook <keescook@chromium.org>

There may still be some subsystems that do not
go through kmalloc for multi-page allocations,
and also do not use __GFP_COMP

I do not know whether there are, but if they exist
those would still trip up the same way SLOB got
tripped up before your patch.

One big question I have for Linus is, do we want
to allow code that does a higher order allocation,
and then frees part of it in smaller orders, or
individual pages, and keeps using the remainder?

>From both a hardening and a simple stability
point of view, allowing memory to be allocated
in one size, and freed in another, seems like
it could be asking for bugs.

If we decide we do not want to allow that,
we can just do the __GFP_COMP markings
unconditionally, and show a big fat warning
when memory gets freed in a different size
than it was allocated.

Is that something we want to do?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] usercopy: Skip multi-page bounds checking on SLOB

[Linus Torvalds]

On Thu, Aug 18, 2016 at 7:21 AM, Rik van Riel <riel@redhat.com> wrote:
>
> One big question I have for Linus is, do we want
> to allow code that does a higher order allocation,
> and then frees part of it in smaller orders, or
> individual pages, and keeps using the remainder?

Yes. We've even had people do that, afaik. IOW, if you know you're
going to allocate 16 pages, you can try to do an order-4 allocation
and just use the 16 pages directly (but still as individual pages),
and avoid extra allocation costs (and to perhaps get better access
patterns if the allocation succeeds etc etc).

That sounds odd, but it actually makes sense when you have the order-4
allocation as a optimistic path (and fall back to doing smaller orders
when a big-order allocation fails). To make that *purely* just an
optimization, you need to let the user then treat that order-4
allocation as individual pages, and free them one by one etc.

So I'm not sure anybody actually does that, but the buddy allocator
was partly designed for that case.

> From both a hardening and a simple stability
> point of view, allowing memory to be allocated
> in one size, and freed in another, seems like
> it could be asking for bugs.

Quite frankly, I'd much rather instead make a hard rule that "user
copies can never be more than one page".

There are *very* few things that actually have bigger user copies, and
we could make those explicitly loop, or mark them as such.

The single-page size limit is fairly natural because of how both our
page cache and our pathname limiting is limited to single pages.

The only thing that generally isn't a single page tends to be:

 - module loading code with vmalloc destination

   We *already* chunk that for other reasons, although we ended up
making the chunk size be 16 pages. Making it a single page wouldn't
really hurt anything.

 - we probably have networking cases that might have big socket buffer
allocations etc.

 - there could be some very strange driver, but we'd find them fairly
quickly if we just start out with making th ecopy_from/to_user()
callers just unconditionally have a

     WARN_ON_ONCE((len) > PAGE_SIZE);

  and not making it fatal, but making it easy to find.

Anyway, I think *that* would be a much easier rule to start with than
worrying about page crossing.

Yes, page crossing can be nasty, and maybe we can try to aim for that
in the future (and mark things that the FPU saving special, because it
really is very very unusual), but I'd actually prefer the 4kB rule
first because that would also allow us to just get rid of the odd
vmalloc special cases etc in the checking.

            Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] usercopy: Skip multi-page bounds checking on SLOB

[Rik van Riel]

On Thu, 2016-08-18 at 10:42 -0700, Linus Torvalds wrote:
> On Thu, Aug 18, 2016 at 7:21 AM, Rik van Riel <riel@redhat.com>
> wrote:
> > 
> > One big question I have for Linus is, do we want
> > to allow code that does a higher order allocation,
> > and then frees part of it in smaller orders, or
> > individual pages, and keeps using the remainder?
> 
> Yes. We've even had people do that, afaik. IOW, if you know you're
> going to allocate 16 pages, you can try to do an order-4 allocation
> and just use the 16 pages directly (but still as individual pages),
> and avoid extra allocation costs (and to perhaps get better access
> patterns if the allocation succeeds etc etc).
> 
> That sounds odd, but it actually makes sense when you have the order-
> 4
> allocation as a optimistic path (and fall back to doing smaller
> orders
> when a big-order allocation fails). To make that *purely* just an
> optimization, you need to let the user then treat that order-4
> allocation as individual pages, and free them one by one etc.
> 
> So I'm not sure anybody actually does that, but the buddy allocator
> was partly designed for that case.

That makes sense. A With that in mind,
it would probably be better to just drop
all of the multi-page bounds checking
from the usercopy code, not conditionally
on SLOB.

Alternatively, we could turn the
__GFP_COMP flag into its negative, and
set it only on the code paths that do
what Linus describes (if anyone does
it).

A WARN_ON_ONCE in the page freeing code
could catch these cases, and point people
at exactly what to do if they trigger the
warning.

I am unclear no how to exclude legitimate
usercopies that are larger than PAGE_SIZE
from triggering warnings/errors, if we
cannot identify every buffer where larger
copies are legitimately going.

Having people rewrite their usercopy code
into loops that automatically avoids
triggering page crossing or >PAGE_SIZE
checks would be counterproductive, since
that might just opens up new attack surface.



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] usercopy: Skip multi-page bounds checking on SLOB

[Kees Cook]

On Thu, Aug 18, 2016 at 11:02 AM, Rik van Riel <riel@redhat.com> wrote:
> On Thu, 2016-08-18 at 10:42 -0700, Linus Torvalds wrote:
>> On Thu, Aug 18, 2016 at 7:21 AM, Rik van Riel <riel@redhat.com>
>> wrote:
>> >
>> > One big question I have for Linus is, do we want
>> > to allow code that does a higher order allocation,
>> > and then frees part of it in smaller orders, or
>> > individual pages, and keeps using the remainder?
>>
>> Yes. We've even had people do that, afaik. IOW, if you know you're
>> going to allocate 16 pages, you can try to do an order-4 allocation
>> and just use the 16 pages directly (but still as individual pages),
>> and avoid extra allocation costs (and to perhaps get better access
>> patterns if the allocation succeeds etc etc).
>>
>> That sounds odd, but it actually makes sense when you have the order-
>> 4
>> allocation as a optimistic path (and fall back to doing smaller
>> orders
>> when a big-order allocation fails). To make that *purely* just an
>> optimization, you need to let the user then treat that order-4
>> allocation as individual pages, and free them one by one etc.
>>
>> So I'm not sure anybody actually does that, but the buddy allocator
>> was partly designed for that case.
>
> That makes sense.  With that in mind,
> it would probably be better to just drop
> all of the multi-page bounds checking
> from the usercopy code, not conditionally
> on SLOB.
>
> Alternatively, we could turn the
> __GFP_COMP flag into its negative, and
> set it only on the code paths that do
> what Linus describes (if anyone does
> it).
>
> A WARN_ON_ONCE in the page freeing code
> could catch these cases, and point people
> at exactly what to do if they trigger the
> warning.
>
> I am unclear no how to exclude legitimate
> usercopies that are larger than PAGE_SIZE
> from triggering warnings/errors, if we
> cannot identify every buffer where larger
> copies are legitimately going.
>
> Having people rewrite their usercopy code
> into loops that automatically avoids
> triggering page crossing or >PAGE_SIZE
> checks would be counterproductive, since
> that might just opens up new attack surface.

Yeah, I agree: we want to have centralized bounds checking and if we
offload >PAGE_SIZE copies to the callers, we're asking for a world of
hurt.

One thing I'm expecting to add in the future is a const-sized
copy_*_user API. This will give us a way to make exceptions to
non-whitelisted slab entries, given that the bounds are const at
compile time. It would behave more like get/put_user in that regard,
but could still handle small exceptions to allocations that would have
been otherwise disallowed (in the forthcoming
HARDENED_USERCOPY_WHITELIST series).

If we encounter another case of a multi-page false positive, we can
just entirely drop that check. For now, let's keep this removed for
SLOB only, and move forward.

-Kees

-- 
Kees Cook
Nexus Security

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] usercopy: Skip multi-page bounds checking on SLOB

[Michal Hocko]

On Thu 18-08-16 10:21:58, Rik van Riel wrote:
> On Wed, 2016-08-17 at 15:29 -0700, Kees Cook wrote:
> > When an allocator does not mark all allocations as PageSlab, or does
> > not
> > mark multipage allocations with __GFP_COMP, hardened usercopy cannot
> > correctly validate the allocation. SLOB lacks this, so short-circuit
> > the checking for the allocators that aren't marked with
> > CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR. This also updates the config
> > help and corrects a typo in the usercopy comments.
> > 
> > Reported-by: xiaolong.ye@intel.com
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> There may still be some subsystems that do not
> go through kmalloc for multi-page allocations,
> and also do not use __GFP_COMP
> 
> I do not know whether there are, but if they exist
> those would still trip up the same way SLOB got
> tripped up before your patch.
> 
> One big question I have for Linus is, do we want
> to allow code that does a higher order allocation,
> and then frees part of it in smaller orders, or
> individual pages, and keeps using the remainder?

We even have an API for that alloc_pages_exact. I do not think anybody
uses that for copying from/to userspace but this pattern is not all that
rare.
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] usercopy: Skip multi-page bounds checking on SLOB

[Linus Torvalds]

On Wed, Aug 17, 2016 at 3:29 PM, Kees Cook <keescook@chromium.org> wrote:
> When an allocator does not mark all allocations as PageSlab, or does not
> mark multipage allocations with __GFP_COMP, hardened usercopy cannot
> correctly validate the allocation. SLOB lacks this, so short-circuit
> the checking for the allocators that aren't marked with
> CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR. This also updates the config
> help and corrects a typo in the usercopy comments.

I think I'm going to instead do just this:

  diff --git a/security/Kconfig b/security/Kconfig
  index df28f2b6f3e1..da10d9b573a4 100644
  --- a/security/Kconfig
  +++ b/security/Kconfig
  @@ -136,6 +136,7 @@ config HAVE_ARCH_HARDENED_USERCOPY
   config HARDENED_USERCOPY
          bool "Harden memory copies between kernel and userspace"
          depends on HAVE_ARCH_HARDENED_USERCOPY
  +       depends on HAVE_HARDENED_USERCOPY_ALLOCATOR
          select BUG
          help
            This option checks for obviously wrong memory regions when

which basically disables the hardened usercopy for SLOB systems.
Nobody cares, because nobody should use SLOB anyway, and certainly
wouldn't use it with hardening.

Let's see if we get any other warnings with that..

              Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] usercopy: Skip multi-page bounds checking on SLOB

[Kees Cook]

On Fri, Aug 19, 2016 at 12:41 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Aug 17, 2016 at 3:29 PM, Kees Cook <keescook@chromium.org> wrote:
>> When an allocator does not mark all allocations as PageSlab, or does not
>> mark multipage allocations with __GFP_COMP, hardened usercopy cannot
>> correctly validate the allocation. SLOB lacks this, so short-circuit
>> the checking for the allocators that aren't marked with
>> CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR. This also updates the config
>> help and corrects a typo in the usercopy comments.
>
> I think I'm going to instead do just this:
>
>   diff --git a/security/Kconfig b/security/Kconfig
>   index df28f2b6f3e1..da10d9b573a4 100644
>   --- a/security/Kconfig
>   +++ b/security/Kconfig
>   @@ -136,6 +136,7 @@ config HAVE_ARCH_HARDENED_USERCOPY
>    config HARDENED_USERCOPY
>           bool "Harden memory copies between kernel and userspace"
>           depends on HAVE_ARCH_HARDENED_USERCOPY
>   +       depends on HAVE_HARDENED_USERCOPY_ALLOCATOR
>           select BUG
>           help
>             This option checks for obviously wrong memory regions when
>
> which basically disables the hardened usercopy for SLOB systems.
> Nobody cares, because nobody should use SLOB anyway, and certainly
> wouldn't use it with hardening.

Okay, I can live with that. I'd hoped to keep the general split
between the other checks (i.e. stack) and the allocator, but if this
is preferred, that's cool. :)

> Let's see if we get any other warnings with that..

Another report came back on NFS root, but it didn't stop the system
from booting, and may be a legit memory exposure report. I'm still
investigating that.

-Kees

-- 
Kees Cook
Nexus Security

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] usercopy: Skip multi-page bounds checking on SLOB

[Linus Torvalds]

On Fri, Aug 19, 2016 at 1:03 PM, Kees Cook <keescook@chromium.org> wrote:
>
> Okay, I can live with that. I'd hoped to keep the general split
> between the other checks (i.e. stack) and the allocator, but if this
> is preferred, that's cool. :)

If it had been anything else than SLOB, I might have cared. As it was,
I didn't think it was worth worrying about SLOB together with
hardening.

It was also about the __check_object_size() modification just being
very ugly, with a "return NULL" in the middle of the function. I
looked at just splitting that function up, and having a part of it
that would just go away when the slab allocator wasn't smart enough,
but that would have been a bigger change that I'm not interested in
taking right now. So it could be a future improvement, and maybe we
could then re-instate SLOB with partial checking.

             Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>