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]

Message-ID: <20170411183035.GD21171@dhcp22.suse.cz>
Date:   Tue, 11 Apr 2017 20:30:36 +0200
From:   Michal Hocko <mhocko@...nel.org>
To:     Christoph Lameter <cl@...ux.com>
Cc:     Kees Cook <keescook@...omium.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        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 Tue 11-04-17 13:03:01, Cristopher Lameter wrote:
> On Tue, 11 Apr 2017, Michal Hocko wrote:
> 
> > >
> > > There is a flag SLAB_DEBUG_OBJECTS that is available for this check.
> >
> > Which is way too late, at least for the kfree path. page->slab_cache
> > on anything else than PageSlab is just a garbage. And my understanding
> > of the patch objective is to stop those from happening.
> 
> We are looking here at SLAB. SLUB code can legitimately have a compound
> page there because large allocations fallback to the page allocator.
> 
> Garbage would be attempting to free a page that has !PageSLAB set but also
> is no compound page. That condition is already checked in kfree() with a
> BUG_ON() and that BUG_ON has been there for a long time.

Are you talking about SLAB or SLUB here?  The only
BUG_ON(PageSlab(page)) in SLAB I can see is in kmem_freepages and that
is way too late because we already rely on cachep which is not
trustworthy. Or am I missing some other place you have in mind?

> Certainly we can
> make SLAB consistent if there is no check there already. Slab just
> attempts a free on that object which will fail too.
> 
> So we are already handling that condition. Why change things? Add a BUG_ON
> if you want to make SLAB consistent.

I hate to repeat myself but let me do it for the last time in this
thread. BUG_ON for something that is recoverable is completely
inappropriate. And I consider kfree with a bogus pointer something that
we can easily recover from. There are other cases where the internal
state of the allocator is compromised to the point where continuing is
not possible and BUGing there is acceptable but kfree(garbage) is not
that case. 

-- 
Michal Hocko
SUSE Labs