linux-kernel - RE: [PATCH] slab: kmalloc_size_roundup() must not return 0 for non-zero size

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] [day] [month] [year] [list]

Message-ID: <c7aa755a2155427ca4cca984efffc791@AcuMS.aculab.com>
Date:   Thu, 7 Sep 2023 08:55:51 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Kees Cook' <keescook@...omium.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "'linux-mm@...ck.org'" <linux-mm@...ck.org>,
        'Vlastimil Babka' <vbabka@...e.cz>,
        'Christoph Lameter' <cl@...ux.com>,
        'Pekka Enberg' <penberg@...nel.org>,
        'David Rientjes' <rientjes@...gle.com>,
        'Joonsoo Kim' <iamjoonsoo.kim@....com>,
        'Andrew Morton' <akpm@...ux-foundation.org>,
        "'Eric Dumazet'" <edumazet@...gle.com>
Subject: RE: [PATCH] slab: kmalloc_size_roundup() must not return 0 for
 non-zero size

From: Kees Cook
> Sent: 06 September 2023 19:17
> 
> On Wed, Sep 06, 2023 at 08:18:21AM +0000, David Laight wrote:
> > The typical use of kmalloc_size_roundup() is:
> > 	ptr = kmalloc(sz = kmalloc_size_roundup(size), ...);
> > 	if (!ptr) return -ENOMEM.
> > This means it is vitally important that the returned value isn't
> > less than the argument even if the argument is insane.
> > In particular if kmalloc_slab() fails or the value is above
> > (MAX_ULONG - PAGE_SIZE) zero is returned and kmalloc() will return
> > it's single zero-length buffer.
> >
> > Fix by returning the input size on error or if the size exceeds
> > a 'sanity' limit.
> > kmalloc() will then return NULL is the size really is too big.
> >
> > Signed-off-by: David Laight <david.laight@...lab.com>
> > Fixes: 05a940656e1eb ("slab: Introduce kmalloc_size_roundup()")
> > ---
> > The 'sanity limit' value doesn't really matter (even if too small)
> > It could be 'MAX_ORDER + PAGE_SHIFT' but one ppc64 has MAX_ORDER 16
> > and I don't know if that also has large pages.
> > Maybe it could be 1ul << 30 on 64bit, but it really doesn't matter
> > if it is too big.
> 
> I agree that returning 0 for an (impossible to reach) non-zero
> is wrong, but the problem seen in netdev was that a truncation happened
> for a value returned by kmalloc_size_roundup().
> 
> So, for the first, it shouldn't be possible for "c" to ever be NULL here:

If it isn't possible there is no need to check :-)

> 
> 	c = kmalloc_slab(size, GFP_KERNEL, 0);
> 	return c ? c->object_size : 0;
> 
> But sure, we can return KMALLOC_MAX_SIZE for that.

Isn't KMALLOC_MAX_SIZE actually valid? - so would be wrong.
Returning 'size' is always valid, the later kmalloc() will
almost certainly fail, but it is also ok if it suceeds.

> The pathological case was this:

s/pathological/failing/

> 
> 	unsigned int truncated;
> 	size_t fullsize = UINT_MAX + 1;
> 
>  	ptr = kmalloc(truncated = kmalloc_size_roundup(fullsize), ...);

The actual pathological case is:
	kmalloc(kmalloc_size_roundup(~0ULL - PAGESIZE/2), ...)
which is kmalloc(0, ...) and suceeds.
	
> Should the logic be changed to return KMALLOC_MAX_SIZE for anything
> larger than KMALLOC_MAX_SIZE? This seems like a different kind of
> foot-gun.
> 
> Everything else in the allocator sanity checking (e.g. struct_size(),
> etc) uses SIZE_MAX as the saturation value, which is why
> kmalloc_size_roundup() did too.

SIZE_MAX (aka ~0ull) seems far too large for sanity checking lengths.
(Even without the issue of having no headroom.)

A limit related to an upper bound for vmalloc() would probably
be more appropriate, or maybe just a limit based on kernel VA.
So for 32bit 2^30 is way too large for any kind of allocate.
For 64bit you can go higher (even if the allocators can't
support the values), maybe 2^48?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)