[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <202309071235.CB4F6B2@keescook>
Date: Thu, 7 Sep 2023 12:38:21 -0700
From: Kees Cook <keescook@...omium.org>
To: David Laight <David.Laight@...lab.com>
Cc: 'Vlastimil Babka' <vbabka@...e.cz>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"'linux-mm@...ck.org'" <linux-mm@...ck.org>,
'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>,
Hyeonggon Yoo <42.hyeyoo@...il.com>,
Roman Gushchin <roman.gushchin@...ux.dev>
Subject: Re: Subject: [PATCH v2] slab: kmalloc_size_roundup() must not return
0 for non-zero size
On Thu, Sep 07, 2023 at 12:42:20PM +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()")
> ---
> v2:
> - Use KMALLOC_MAX_SIZE for upper limit.
> (KMALLOC_MAX_SIZE + 1 may give better code on some archs!)
> - Invert test for overlarge for consistency.
> - Put a likely() on result of kmalloc_slab().
>
> mm/slab_common.c | 26 +++++++++++++-------------
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index cd71f9581e67..0fb7c7e19bad 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -747,22 +747,22 @@ size_t kmalloc_size_roundup(size_t size)
> {
> struct kmem_cache *c;
>
> - /* Short-circuit the 0 size case. */
> - if (unlikely(size == 0))
> - return 0;
If we want to allow 0, let's just leave this case as-is: the compiler
will optimize it against the other tests.
> - /* Short-circuit saturated "too-large" case. */
> - if (unlikely(size == SIZE_MAX))
> - return SIZE_MAX;
> + if (size && size <= KMALLOC_MAX_CACHE_SIZE) {
> + /*
> + * The flags don't matter since size_index is common to all.
> + * Neither does the caller for just getting ->object_size.
> + */
> + c = kmalloc_slab(size, GFP_KERNEL, 0);
> + return likely(c) ? c->object_size : size;
I would like to have this fail "safe". c should never be NULL here, so
let's return "KMALLOC_MAX_SIZE + 1" to force failures.
> + }
> +
> /* Above the smaller buckets, size is a multiple of page size. */
> - if (size > KMALLOC_MAX_CACHE_SIZE)
> + if (size && size <= KMALLOC_MAX_SIZE)
> return PAGE_SIZE << get_order(size);
>
> - /*
> - * The flags don't matter since size_index is common to all.
> - * Neither does the caller for just getting ->object_size.
> - */
> - c = kmalloc_slab(size, GFP_KERNEL, 0);
> - return c ? c->object_size : 0;
> + /* Return 'size' for 0 and very large - kmalloc() may fail. */
I want to _be certain_ failure happens. If we get here we need to return
"KMALLOC_MAX_SIZE + 1"
-Kees
> + return size;
> +
> }
> EXPORT_SYMBOL(kmalloc_size_roundup);
>
> --
> 2.17.1
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
--
Kees Cook
Powered by blists - more mailing lists