Re: [PATCH 1/1] null_blk: Use u64 to avoid overflow in null_alloc_dev()

[Damien Le Moal <dlemoal@kernel.org>]

On 2024/09/22 10:59, Zhu Yanjun wrote:
> The member variable size in struct nullb_device is the type
> unsigned long, and the module parameter g_gb is the type int.
> In 32 bit architecture, unsigned long has 32 bit. This
> introduces overflow risks.
> 
> Use the type u64 in struct nullb_device and configfs. This
> can avoid overflow risks.
> 
> Fixes: 2984c8684f96 ("nullb: factor disk parameters")
> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
> ---
>  drivers/block/null_blk/main.c     | 23 +++++++++++++++++++++--
>  drivers/block/null_blk/null_blk.h |  2 +-
>  2 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> index 2f0431e42c49..88c6d6277d09 100644
> --- a/drivers/block/null_blk/main.c
> +++ b/drivers/block/null_blk/main.c
> @@ -289,6 +289,11 @@ static inline ssize_t nullb_device_ulong_attr_show(unsigned long val,
>  	return snprintf(page, PAGE_SIZE, "%lu\n", val);
>  }
>  
> +static inline ssize_t nullb_device_u64_attr_show(u64 val, char *page)
> +{
> +	return snprintf(page, PAGE_SIZE, "%llu\n", val);
> +}
> +
>  static inline ssize_t nullb_device_bool_attr_show(bool val, char *page)
>  {
>  	return snprintf(page, PAGE_SIZE, "%u\n", val);
> @@ -322,6 +327,20 @@ static ssize_t nullb_device_ulong_attr_store(unsigned long *val,
>  	return count;
>  }
>  
> +static ssize_t nullb_device_u64_attr_store(u64 *val, const char *page,
> +	size_t count)
> +{
> +	int result;
> +	u64 tmp;
> +
> +	result = kstrtou64(page, 0, &tmp);
> +	if (result < 0)
> +		return result;
> +
> +	*val = tmp;
> +	return count;
> +}
> +
>  static ssize_t nullb_device_bool_attr_store(bool *val, const char *page,
>  	size_t count)
>  {
> @@ -438,7 +457,7 @@ static int nullb_apply_poll_queues(struct nullb_device *dev,
>  	return ret;
>  }
>  
> -NULLB_DEVICE_ATTR(size, ulong, NULL);
> +NULLB_DEVICE_ATTR(size, u64, NULL);
>  NULLB_DEVICE_ATTR(completion_nsec, ulong, NULL);
>  NULLB_DEVICE_ATTR(submit_queues, uint, nullb_apply_submit_queues);
>  NULLB_DEVICE_ATTR(poll_queues, uint, nullb_apply_poll_queues);
> @@ -762,7 +781,7 @@ static struct nullb_device *null_alloc_dev(void)
>  		return NULL;
>  	}
>  
> -	dev->size = g_gb * 1024;
> +	dev->size = (u64)g_gb * 1024;

As already commented on your previous version that was casting to an unsigned
long, this is *not* avoiding an overflow. It is only changing the overflow value
to a bigger one. So as suggested before, if you really want to fix this, fix it
properly using check_mul_overflow().


-- 
Damien Le Moal
Western Digital Research