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

[Yu Kuai <yukuai1@xxxxxxxxxxxxxxx>]

Hi, Damien

在 2024/09/23 16:11, Damien Le Moal 写道:
> 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@xxxxxxxxx>
> > ---
> >   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().

g_gb is 32-bit value in GB, hence the result won't overflow if size
is changed to 64-bit value now.

And please also use a specific store function instead of
nullb_device_u64_attr_store(), and using check_mul_overflow() to check
overflow, because dev->size will used later for inode size, and 64-bit
value in MB can overflow after switching to 64-bit value in sectors.

Thanks,
Kuai

>