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

Groups > linux.kernel > #1614146 > unrolled thread

[PATCH] mm: Add additional consistency check

Back to article view | Back to linux.kernel

Contents

  [PATCH] mm: Add additional consistency check Kees Cook <keescook@chromium.org> - 2017-03-31 18:50 +0200
    Re: [PATCH] mm: Add additional consistency check Andrew Morton <akpm@linux-foundation.org> - 2017-03-31 23:40 +0200
      Re: [PATCH] mm: Add additional consistency check Kees Cook <keescook@chromium.org> - 2017-04-01 02:10 +0200
        Re: [PATCH] mm: Add additional consistency check Michael Ellerman <mpe@ellerman.id.au> - 2017-04-03 05:50 +0200
          Re: [PATCH] mm: Add additional consistency check Christoph Lameter <cl@linux.com> - 2017-04-03 16:20 +0200
            Re: [PATCH] mm: Add additional consistency check Matthew Wilcox <willy@infradead.org> - 2017-04-03 17:00 +0200
    Re: [PATCH] mm: Add additional consistency check Michal Hocko <mhocko@kernel.org> - 2017-04-04 13:40 +0200
      Re: [PATCH] mm: Add additional consistency check Christoph Lameter <cl@linux.com> - 2017-04-04 17:10 +0200
        Re: [PATCH] mm: Add additional consistency check Michal Hocko <mhocko@kernel.org> - 2017-04-04 17:20 +0200
          Re: [PATCH] mm: Add additional consistency check Kees Cook <keescook@chromium.org> - 2017-04-04 17:50 +0200
            Re: [PATCH] mm: Add additional consistency check Michal Hocko <mhocko@kernel.org> - 2017-04-04 18:00 +0200
              Re: [PATCH] mm: Add additional consistency check Kees Cook <keescook@chromium.org> - 2017-04-04 18:10 +0200
          Re: [PATCH] mm: Add additional consistency check Christoph Lameter <cl@linux.com> - 2017-04-04 21:20 +0200
            Re: [PATCH] mm: Add additional consistency check Michal Hocko <mhocko@kernel.org> - 2017-04-04 21:50 +0200
              Re: [PATCH] mm: Add additional consistency check Christoph Lameter <cl@linux.com> - 2017-04-04 22:00 +0200
                Re: [PATCH] mm: Add additional consistency check Michal Hocko <mhocko@kernel.org> - 2017-04-04 22:20 +0200

#1614146 — [PATCH] mm: Add additional consistency check

As found in PaX, this adds a cheap check on heap consistency, just to
notice if things have gotten corrupted in the page lookup.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 mm/slab.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/slab.h b/mm/slab.h
index 65e7c3fcac72..64447640b70c 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -384,6 +384,7 @@ static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
 		return s;
 
 	page = virt_to_head_page(x);
+	BUG_ON(!PageSlab(page));
 	cachep = page->slab_cache;
 	if (slab_equal_or_root(cachep, s))
 		return cachep;
-- 
2.7.4


-- 
Kees Cook
Pixel Security

[toc] | [next] | [standalone]

#1614299

On Fri, 31 Mar 2017 09:40:28 -0700 Kees Cook <keescook@chromium.org> wrote:

> As found in PaX, this adds a cheap check on heap consistency, just to
> notice if things have gotten corrupted in the page lookup.

"As found in PaX" isn't a very illuminating justification for such a
change.  Was there a real kernel bug which this would have exposed, or
what?

> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -384,6 +384,7 @@ static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
>  		return s;
>  
>  	page = virt_to_head_page(x);
> +	BUG_ON(!PageSlab(page));
>  	cachep = page->slab_cache;
>  	if (slab_equal_or_root(cachep, s))
>  		return cachep;

BUG_ON might be too severe.  I expect the kindest VM_WARN_ON_ONCE()
would suffice here, but without more details it is hard to say.

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

#1614343

On Fri, Mar 31, 2017 at 2:33 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Fri, 31 Mar 2017 09:40:28 -0700 Kees Cook <keescook@chromium.org> wrote:
>
>> As found in PaX, this adds a cheap check on heap consistency, just to
>> notice if things have gotten corrupted in the page lookup.
>
> "As found in PaX" isn't a very illuminating justification for such a
> change.  Was there a real kernel bug which this would have exposed, or
> what?

I don't know off the top of my head, but given the kinds of heap
attacks I've been seeing, I think this added consistency check is
worth it given how inexpensive it is. When heap metadata gets
corrupted, we can get into nasty side-effects that can be
attacker-controlled, so better to catch obviously bad states as early
as possible.

>> --- a/mm/slab.h
>> +++ b/mm/slab.h
>> @@ -384,6 +384,7 @@ static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
>>               return s;
>>
>>       page = virt_to_head_page(x);
>> +     BUG_ON(!PageSlab(page));
>>       cachep = page->slab_cache;
>>       if (slab_equal_or_root(cachep, s))
>>               return cachep;
>
> BUG_ON might be too severe.  I expect the kindest VM_WARN_ON_ONCE()
> would suffice here, but without more details it is hard to say.

So, WARN isn't enough to protect the kernel (execution continues and
the memory is still dereferenced for malicious purposes, etc). Perhaps
use CHECK_DATA_CORRUPTION() here, which can either WARN and take a
"safe" path, or BUG (depending on config paranoia of the builder).
I've got a series adding it in a number of other places, so I could
add this patch to that series?

-Kees

-- 
Kees Cook
Pixel Security

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

#1614874

Kees Cook <keescook@chromium.org> writes:

> On Fri, Mar 31, 2017 at 2:33 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
>> On Fri, 31 Mar 2017 09:40:28 -0700 Kees Cook <keescook@chromium.org> wrote:
>>
>>> As found in PaX, this adds a cheap check on heap consistency, just to
>>> notice if things have gotten corrupted in the page lookup.
>>
>> "As found in PaX" isn't a very illuminating justification for such a
>> change.  Was there a real kernel bug which this would have exposed, or
>> what?
>
> I don't know off the top of my head, but given the kinds of heap
> attacks I've been seeing, I think this added consistency check is
> worth it given how inexpensive it is. When heap metadata gets
> corrupted, we can get into nasty side-effects that can be
> attacker-controlled, so better to catch obviously bad states as early
> as possible.

There's your changelog :)

>>> --- a/mm/slab.h
>>> +++ b/mm/slab.h
>>> @@ -384,6 +384,7 @@ static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
>>>               return s;
>>>
>>>       page = virt_to_head_page(x);
>>> +     BUG_ON(!PageSlab(page));
>>>       cachep = page->slab_cache;
>>>       if (slab_equal_or_root(cachep, s))
>>>               return cachep;
>>
>> BUG_ON might be too severe.  I expect the kindest VM_WARN_ON_ONCE()
>> would suffice here, but without more details it is hard to say.
>
> So, WARN isn't enough to protect the kernel (execution continues and
> the memory is still dereferenced for malicious purposes, etc).

You could do:

	if (WARN_ON(!PageSlab(page)))
        	return NULL.

Though I see at least two callers that don't check for a NULL return.

Looking at the context, the tail of the function already contains:

	pr_err("%s: Wrong slab cache. %s but object is from %s\n",
	       __func__, s->name, cachep->name);
	WARN_ON_ONCE(1);
	return s;
}

At least in slab.c it seems that would allow you to "free" an object
from one kmem_cache onto the array_cache of another kmem_cache, which
seems fishy. But maybe there's a check somewhere I'm missing?

cheers

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

#1615231

On Mon, 3 Apr 2017, Michael Ellerman wrote:

> At least in slab.c it seems that would allow you to "free" an object
> from one kmem_cache onto the array_cache of another kmem_cache, which
> seems fishy. But maybe there's a check somewhere I'm missing?

kfree can be used to free any object from any slab cache.

kmem_cache_free() checks if the object belongs to the cache given.

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

#1615291

On Mon, Apr 03, 2017 at 09:03:50AM -0500, Christoph Lameter wrote:
> On Mon, 3 Apr 2017, Michael Ellerman wrote:
> 
> > At least in slab.c it seems that would allow you to "free" an object
> > from one kmem_cache onto the array_cache of another kmem_cache, which
> > seems fishy. But maybe there's a check somewhere I'm missing?
> 
> kfree can be used to free any object from any slab cache.

Is that a guarantee?  There's some wording in the RCU free code that
seems to indicate we can't rely on that being true.

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

#1615926

On Fri 31-03-17 09:40:28, Kees Cook wrote:
> As found in PaX, this adds a cheap check on heap consistency, just to
> notice if things have gotten corrupted in the page lookup.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>

NAK without a proper changelog. Seriously, we do not blindly apply
changes from other projects without a deep understanding of all
consequences.

> ---
>  mm/slab.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/mm/slab.h b/mm/slab.h
> index 65e7c3fcac72..64447640b70c 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -384,6 +384,7 @@ static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
>  		return s;
>  
>  	page = virt_to_head_page(x);
> +	BUG_ON(!PageSlab(page));
>  	cachep = page->slab_cache;
>  	if (slab_equal_or_root(cachep, s))
>  		return cachep;
> -- 
> 2.7.4
> 
> 
> -- 
> Kees Cook
> Pixel 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>

-- 
Michal Hocko
SUSE Labs

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

#1616091

On Tue, 4 Apr 2017, Michal Hocko wrote:

> NAK without a proper changelog. Seriously, we do not blindly apply
> changes from other projects without a deep understanding of all
> consequences.

Functionalitywise this is trivial. A page must be a slab page in order to
be able to determine the slab cache of an object. Its definitely not ok if
the page is not a slab page.

The main issue that may exist here is the adding of overhead to a critical
code path like kfree().

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

#1616093

On Tue 04-04-17 10:07:23, Cristopher Lameter wrote:
> On Tue, 4 Apr 2017, Michal Hocko wrote:
> 
> > NAK without a proper changelog. Seriously, we do not blindly apply
> > changes from other projects without a deep understanding of all
> > consequences.
> 
> Functionalitywise this is trivial. A page must be a slab page in order to
> be able to determine the slab cache of an object. Its definitely not ok if
> the page is not a slab page.

Yes, but we do not have to blow the kernel, right? Why cannot we simply
leak that memory?

> The main issue that may exist here is the adding of overhead to a critical
> code path like kfree().

Yes, nothing is for free. But if the attack space is real then we
probably want to sacrifice few cycles (to simply return ASAP without
further further processing). This all should be in the changelog ideally
with some numbers. I suspect this would be hard to measure in most
workloads.

-- 
Michal Hocko
SUSE Labs

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

#1616130

On Tue, Apr 4, 2017 at 8:16 AM, Michal Hocko <mhocko@kernel.org> wrote:
> On Tue 04-04-17 10:07:23, Cristopher Lameter wrote:
>> On Tue, 4 Apr 2017, Michal Hocko wrote:
>>
>> > NAK without a proper changelog. Seriously, we do not blindly apply
>> > changes from other projects without a deep understanding of all
>> > consequences.
>>
>> Functionalitywise this is trivial. A page must be a slab page in order to
>> be able to determine the slab cache of an object. Its definitely not ok if
>> the page is not a slab page.
>
> Yes, but we do not have to blow the kernel, right? Why cannot we simply
> leak that memory?

I can put this behind CHECK_DATA_CORRUPTION() instead of BUG(), which
allows the system builder to choose between WARN and BUG. Some people
absolutely want the kernel to BUG on data corruption as it could be an
attack.

>> The main issue that may exist here is the adding of overhead to a critical
>> code path like kfree().
>
> Yes, nothing is for free. But if the attack space is real then we
> probably want to sacrifice few cycles (to simply return ASAP without
> further further processing). This all should be in the changelog ideally
> with some numbers. I suspect this would be hard to measure in most
> workloads.

Given the trivial nature of the check, yeah, it seemed impossible to
actually show performance changes.

-Kees

-- 
Kees Cook
Pixel Security

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

#1616145

On Tue 04-04-17 08:46:02, Kees Cook wrote:
> On Tue, Apr 4, 2017 at 8:16 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > On Tue 04-04-17 10:07:23, Cristopher Lameter wrote:
> >> On Tue, 4 Apr 2017, Michal Hocko wrote:
> >>
> >> > NAK without a proper changelog. Seriously, we do not blindly apply
> >> > changes from other projects without a deep understanding of all
> >> > consequences.
> >>
> >> Functionalitywise this is trivial. A page must be a slab page in order to
> >> be able to determine the slab cache of an object. Its definitely not ok if
> >> the page is not a slab page.
> >
> > Yes, but we do not have to blow the kernel, right? Why cannot we simply
> > leak that memory?
> 
> I can put this behind CHECK_DATA_CORRUPTION() instead of BUG(), which
> allows the system builder to choose between WARN and BUG. Some people
> absolutely want the kernel to BUG on data corruption as it could be an
> attack.

CHECK_DATA_CORRUPTION sounds as better fit to me. This would, however
require to handle the potenial corruption by returning and leaking the
memory.

-- 
Michal Hocko
SUSE Labs

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

#1616157

On Tue, Apr 4, 2017 at 8:58 AM, Michal Hocko <mhocko@kernel.org> wrote:
> On Tue 04-04-17 08:46:02, Kees Cook wrote:
>> On Tue, Apr 4, 2017 at 8:16 AM, Michal Hocko <mhocko@kernel.org> wrote:
>> > On Tue 04-04-17 10:07:23, Cristopher Lameter wrote:
>> >> On Tue, 4 Apr 2017, Michal Hocko wrote:
>> >>
>> >> > NAK without a proper changelog. Seriously, we do not blindly apply
>> >> > changes from other projects without a deep understanding of all
>> >> > consequences.
>> >>
>> >> Functionalitywise this is trivial. A page must be a slab page in order to
>> >> be able to determine the slab cache of an object. Its definitely not ok if
>> >> the page is not a slab page.
>> >
>> > Yes, but we do not have to blow the kernel, right? Why cannot we simply
>> > leak that memory?
>>
>> I can put this behind CHECK_DATA_CORRUPTION() instead of BUG(), which
>> allows the system builder to choose between WARN and BUG. Some people
>> absolutely want the kernel to BUG on data corruption as it could be an
>> attack.
>
> CHECK_DATA_CORRUPTION sounds as better fit to me. This would, however
> require to handle the potenial corruption by returning and leaking the
> memory.

IIUC, that would be the "return s" path? I should likely change the
WARN_ON_ONCE there to be CHECK_DATA_CORRUPTION too. I'll add this to
my series.

-Kees

-- 
Kees Cook
Pixel Security

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

#1616344

On Tue, 4 Apr 2017, Michal Hocko wrote:

> Yes, but we do not have to blow the kernel, right? Why cannot we simply
> leak that memory?

Because it is a serious bug to attempt to free a non slab object using
slab operations. This is often the result of memory corruption, coding
errs etc. The system needs to stop right there.

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

#1616361

On Tue 04-04-17 14:13:06, Cristopher Lameter wrote:
> On Tue, 4 Apr 2017, Michal Hocko wrote:
> 
> > Yes, but we do not have to blow the kernel, right? Why cannot we simply
> > leak that memory?
> 
> Because it is a serious bug to attempt to free a non slab object using
> slab operations. This is often the result of memory corruption, coding
> errs etc. The system needs to stop right there.

Why when an alternative is a memory leak?

-- 
Michal Hocko
SUSE Labs

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

#1616365

On Tue, 4 Apr 2017, Michal Hocko wrote:

> On Tue 04-04-17 14:13:06, Cristopher Lameter wrote:
> > On Tue, 4 Apr 2017, Michal Hocko wrote:
> >
> > > Yes, but we do not have to blow the kernel, right? Why cannot we simply
> > > leak that memory?
> >
> > Because it is a serious bug to attempt to free a non slab object using
> > slab operations. This is often the result of memory corruption, coding
> > errs etc. The system needs to stop right there.
>
> Why when an alternative is a memory leak?

Because the slab allocators fail also in case you free an object multiple
times etc etc. Continuation is supported by enabling a special resiliency
feature via the kernel command line. The alternative is selectable but not
the default.

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

#1616369

On Tue 04-04-17 14:58:06, Cristopher Lameter wrote:
> On Tue, 4 Apr 2017, Michal Hocko wrote:
> 
> > On Tue 04-04-17 14:13:06, Cristopher Lameter wrote:
> > > On Tue, 4 Apr 2017, Michal Hocko wrote:
> > >
> > > > Yes, but we do not have to blow the kernel, right? Why cannot we simply
> > > > leak that memory?
> > >
> > > Because it is a serious bug to attempt to free a non slab object using
> > > slab operations. This is often the result of memory corruption, coding
> > > errs etc. The system needs to stop right there.
> >
> > Why when an alternative is a memory leak?
> 
> Because the slab allocators fail also in case you free an object multiple
> times etc etc. Continuation is supported by enabling a special resiliency
> feature via the kernel command line. The alternative is selectable but not
> the default.
 
I disagree! We should try to continue as long as we _know_ that the
internal state of the allocator is still consistent and a further
operation will not spread the corruption even more. This is clearly not
the case for an invalid pointer to kfree.

I can see why checking for an early allocator corruption is not always
feasible and you can only detect after-the-fact but this is not the case
here and putting your system down just because some buggy code is trying
to free something it hasn't allocated is not really useful. I completely
agree with Linus that we overuse BUG way too much and this is just
another example of it.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web