Re: [PATCH] mm: Add additional consistency check

[Kees Cook <keescook@chromium.org>]

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