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

From: Kees Cook <keescook@chromium.org>
To: David Laight <David.Laight@aculab.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"'linux-mm@kvack.org'" <linux-mm@kvack.org>,
	'Vlastimil Babka' <vbabka@suse.cz>,
	'Christoph Lameter' <cl@linux.com>,
	'Pekka Enberg' <penberg@kernel.org>,
	'David Rientjes' <rientjes@google.com>,
	'Joonsoo Kim' <iamjoonsoo.kim@lge.com>,
	'Andrew Morton' <akpm@linux-foundation.org>,
	'Eric Dumazet' <edumazet@google.com>
Subject: Re: [PATCH] slab: kmalloc_size_roundup() must not return 0 for non-zero size
Date: Wed, 6 Sep 2023 11:16:30 -0700	[thread overview]
Message-ID: <202309061106.C0690BDBB@keescook> (raw)
In-Reply-To: <fcfee37ead054de19871139167aca787@AcuMS.aculab.com>

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@aculab.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:

	c = kmalloc_slab(size, GFP_KERNEL, 0);
	return c ? c->object_size : 0;

But sure, we can return KMALLOC_MAX_SIZE for that.

The pathological case was this:

	unsigned int truncated;
	size_t fullsize = UINT_MAX + 1;

 	ptr = kmalloc(truncated = kmalloc_size_roundup(fullsize), ...);

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.

-- 
Kees Cook