linux-kernel - Re: kmem_cache_attr (was Re: [PATCH 04/36] usercopy: Prepare for usercopy whitelisting)

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: <20180116174315.GA10461@bombadil.infradead.org>
Date:   Tue, 16 Jan 2018 09:43:15 -0800
From:   Matthew Wilcox <willy@...radead.org>
To:     Christopher Lameter <cl@...ux.com>
Cc:     Kees Cook <keescook@...omium.org>, linux-kernel@...r.kernel.org,
        David Windsor <dave@...lcore.net>,
        Pekka Enberg <penberg@...nel.org>,
        David Rientjes <rientjes@...gle.com>,
        Joonsoo Kim <iamjoonsoo.kim@....com>,
        Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
        linux-xfs@...r.kernel.org,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Andy Lutomirski <luto@...nel.org>,
        Christoph Hellwig <hch@...radead.org>,
        "David S. Miller" <davem@...emloft.net>,
        Laura Abbott <labbott@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Christian Borntraeger <borntraeger@...ibm.com>,
        Christoffer Dall <christoffer.dall@...aro.org>,
        Dave Kleikamp <dave.kleikamp@...cle.com>,
        Jan Kara <jack@...e.cz>,
        Luis de Bethencourt <luisbg@...nel.org>,
        Marc Zyngier <marc.zyngier@....com>,
        Rik van Riel <riel@...hat.com>,
        Matthew Garrett <mjg59@...gle.com>,
        linux-fsdevel@...r.kernel.org, linux-arch@...r.kernel.org,
        netdev@...r.kernel.org, kernel-hardening@...ts.openwall.com
Subject: Re: kmem_cache_attr (was Re: [PATCH 04/36] usercopy: Prepare for
 usercopy whitelisting)

On Tue, Jan 16, 2018 at 10:54:27AM -0600, Christopher Lameter wrote:
> On Tue, 16 Jan 2018, Matthew Wilcox wrote:
> 
> > I think that's a good thing!  /proc/slabinfo really starts to get grotty
> > above 16 bytes.  I'd like to chop off "_cache" from the name of every
> > single slab!  If ext4_allocation_context has to become ext4_alloc_ctx,
> > I don't think we're going to lose any valuable information.
> 
> Ok so we are going to cut off at 16 charaacters? Sounds good to me.

Excellent!

> > > struct kmem_cache_attr {
> > > 	char *name;
> > > 	size_t size;
> > > 	size_t align;
> > > 	slab_flags_t flags;
> > > 	unsigned int useroffset;
> > > 	unsinged int usersize;
> > > 	void (*ctor)(void *);
> > > 	kmem_isolate_func *isolate;
> > > 	kmem_migrate_func *migrate;
> > > 	...
> > > }
> >
> > In these slightly-more-security-conscious days, it's considered poor
> > practice to have function pointers in writable memory.  That was why
> > I wanted to make the kmem_cache_attr const.
> 
> Sure this data is never changed. It can be const.

It's changed at initialisation.  Look:

kmem_cache_create(const char *name, size_t size, size_t align,
                  slab_flags_t flags, void (*ctor)(void *))
        s = create_cache(cache_name, size, size,
                         calculate_alignment(flags, align, size),
                         flags, ctor, NULL, NULL);

The 'align' that ends up in s->align, is not the user-specified align.
It's also dependent on runtime information (cache_line_size()), so it
can't be calculated at compile time.

'flags' also gets mangled:
        flags &= CACHE_CREATE_MASK;


> I am not married to either way of specifying the sizes. unsigned int would
> be fine with me. SLUB falls back to the page allocator anyways for
> anything above 2* PAGE_SIZE and I think we can do the same for the other
> allocators as well. Zeroing or initializing such a large memory chunk is
> much more expensive than the allocation so it does not make much sense to
> have that directly supported in the slab allocators.

The only slabs larger than 4kB on my system right now are:
kvm_vcpu               0      0  19136    1    8 : tunables    8    4    0 : slabdata      0      0      0
net_namespace          1      1   6080    1    2 : tunables    8    4    0 : slabdata      1      1      0

(other than the fake slabs for kmalloc)

> Some platforms support 64K page size and I could envision a 2M page size
> at some point. So I think we cannot use 16 bits there.
> 
> If no one objects then I can use unsigned int there again.

unsigned int would be my preference.