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

[Kees Cook]

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