| Message ID | 20240922085941.14832-1-yanjun.zhu@linux.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] null_blk: Use u64 to avoid overflow in null_alloc_dev() | expand |
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().
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@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(). 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 > >
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; dev->completion_nsec = g_completion_nsec; dev->submit_queues = g_submit_queues; dev->prev_submit_queues = g_submit_queues; diff --git a/drivers/block/null_blk/null_blk.h b/drivers/block/null_blk/null_blk.h index a7bb32f73ec3..e30c011909ad 100644 --- a/drivers/block/null_blk/null_blk.h +++ b/drivers/block/null_blk/null_blk.h @@ -74,7 +74,7 @@ struct nullb_device { bool need_zone_res_mgmt; spinlock_t zone_res_lock; - unsigned long size; /* device size in MB */ + u64 size; /* device size in MB */ unsigned long completion_nsec; /* time in ns to complete a request */ unsigned long cache_size; /* disk cache size in MB */ unsigned long zone_size; /* zone size in MB if device is zoned */
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(-)