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:   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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ