linux-kernel - Re: [PATCH] mm: Add additional consistency check

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 31 Mar 2017 17:04:00 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     Christoph Lameter <cl@...ux.com>,
        Pekka Enberg <penberg@...nel.org>,
        David Rientjes <rientjes@...gle.com>,
        Joonsoo Kim <iamjoonsoo.kim@....com>,
        Linux-MM <linux-mm@...ck.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mm: Add additional consistency check

On Fri, Mar 31, 2017 at 2:33 PM, Andrew Morton
<akpm@...ux-foundation.org> wrote:
> On Fri, 31 Mar 2017 09:40:28 -0700 Kees Cook <keescook@...omium.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