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@xxxxxxxxxx>
> 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
