| Message ID | 20240917070729.15752-1-amishin@t-argos.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nullb: Adjust device size calculation in null_alloc_dev() | expand |
在 2024/9/17 15:07, Aleksandr Mishin 写道: > In null_alloc_dev() device size is a subject to overflow because 'g_gb' > (which is module parameter, may have any value and is not validated > anywhere) is not cast to a larger data type before performing arithmetic. > > Cast 'g_gb' to unsigned long to prevent overflow. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Fixes: 2984c8684f96 ("nullb: factor disk parameters") > Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> > --- > drivers/block/null_blk/main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c > index 2f0431e42c49..5edbf9c0aceb 100644 > --- a/drivers/block/null_blk/main.c > +++ b/drivers/block/null_blk/main.c > @@ -762,7 +762,7 @@ static struct nullb_device *null_alloc_dev(void) > return NULL; > } > > - dev->size = g_gb * 1024; The member variable size in struct nullb_device is type "unsigned long". As such, changing g_gb from int to "unsigned long" seems reasonable. So I am fine with this. Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev> Thanks a lot. Zhu Yanjun > + dev->size = (unsigned long)g_gb * 1024; > dev->completion_nsec = g_completion_nsec; > dev->submit_queues = g_submit_queues; > dev->prev_submit_queues = g_submit_queues;
On 2024/09/17 16:07, Aleksandr Mishin wrote: > In null_alloc_dev() device size is a subject to overflow because 'g_gb' > (which is module parameter, may have any value and is not validated > anywhere) is not cast to a larger data type before performing arithmetic. > > Cast 'g_gb' to unsigned long to prevent overflow. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Fixes: 2984c8684f96 ("nullb: factor disk parameters") > Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> > --- > drivers/block/null_blk/main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c > index 2f0431e42c49..5edbf9c0aceb 100644 > --- a/drivers/block/null_blk/main.c > +++ b/drivers/block/null_blk/main.c > @@ -762,7 +762,7 @@ static struct nullb_device *null_alloc_dev(void) > return NULL; > } > > - dev->size = g_gb * 1024; > + dev->size = (unsigned long)g_gb * 1024; This still does not prevent overflows... So what about doing a proper check ? > dev->completion_nsec = g_completion_nsec; > dev->submit_queues = g_submit_queues; > dev->prev_submit_queues = g_submit_queues;
On 2024/09/17 16:21, Damien Le Moal wrote: > On 2024/09/17 16:07, Aleksandr Mishin wrote: >> In null_alloc_dev() device size is a subject to overflow because 'g_gb' >> (which is module parameter, may have any value and is not validated >> anywhere) is not cast to a larger data type before performing arithmetic. >> >> Cast 'g_gb' to unsigned long to prevent overflow. >> >> Found by Linux Verification Center (linuxtesting.org) with SVACE. >> >> Fixes: 2984c8684f96 ("nullb: factor disk parameters") >> Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> >> --- >> drivers/block/null_blk/main.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c >> index 2f0431e42c49..5edbf9c0aceb 100644 >> --- a/drivers/block/null_blk/main.c >> +++ b/drivers/block/null_blk/main.c >> @@ -762,7 +762,7 @@ static struct nullb_device *null_alloc_dev(void) >> return NULL; >> } >> >> - dev->size = g_gb * 1024; >> + dev->size = (unsigned long)g_gb * 1024; > > This still does not prevent overflows... So what about doing a proper check ? This still does not prevent overflows on 32-bits architectures. > >> dev->completion_nsec = g_completion_nsec; >> dev->submit_queues = g_submit_queues; >> dev->prev_submit_queues = g_submit_queues; >
在 2024/9/17 15:24, Damien Le Moal 写道: > On 2024/09/17 16:21, Damien Le Moal wrote: >> On 2024/09/17 16:07, Aleksandr Mishin wrote: >>> In null_alloc_dev() device size is a subject to overflow because 'g_gb' >>> (which is module parameter, may have any value and is not validated >>> anywhere) is not cast to a larger data type before performing arithmetic. >>> >>> Cast 'g_gb' to unsigned long to prevent overflow. >>> >>> Found by Linux Verification Center (linuxtesting.org) with SVACE. >>> >>> Fixes: 2984c8684f96 ("nullb: factor disk parameters") >>> Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> >>> --- >>> drivers/block/null_blk/main.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c >>> index 2f0431e42c49..5edbf9c0aceb 100644 >>> --- a/drivers/block/null_blk/main.c >>> +++ b/drivers/block/null_blk/main.c >>> @@ -762,7 +762,7 @@ static struct nullb_device *null_alloc_dev(void) >>> return NULL; >>> } >>> >>> - dev->size = g_gb * 1024; >>> + dev->size = (unsigned long)g_gb * 1024; >> This still does not prevent overflows... So what about doing a proper check ? > This still does not prevent overflows on 32-bits architectures. The max value of "unsigned long" is 2^64 - 1 while the max value of int is 2^31 -1. (2^64 - 1) / (2^31-1) is about 2^33 while 1024 is 2^10. 2^33 is greater than 2^10. So in the above, it seems that it is difficult to overflow. If I am missing something, please let me know. Thanks, Zhu Yanjun > >>> dev->completion_nsec = g_completion_nsec; >>> dev->submit_queues = g_submit_queues; >>> dev->prev_submit_queues = g_submit_queues;
在 2024/9/17 15:24, Damien Le Moal 写道: > On 2024/09/17 16:21, Damien Le Moal wrote: >> On 2024/09/17 16:07, Aleksandr Mishin wrote: >>> In null_alloc_dev() device size is a subject to overflow because 'g_gb' >>> (which is module parameter, may have any value and is not validated >>> anywhere) is not cast to a larger data type before performing arithmetic. >>> >>> Cast 'g_gb' to unsigned long to prevent overflow. >>> >>> Found by Linux Verification Center (linuxtesting.org) with SVACE. >>> >>> Fixes: 2984c8684f96 ("nullb: factor disk parameters") >>> Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> >>> --- >>> drivers/block/null_blk/main.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c >>> index 2f0431e42c49..5edbf9c0aceb 100644 >>> --- a/drivers/block/null_blk/main.c >>> +++ b/drivers/block/null_blk/main.c >>> @@ -762,7 +762,7 @@ static struct nullb_device *null_alloc_dev(void) >>> return NULL; >>> } >>> >>> - dev->size = g_gb * 1024; >>> + dev->size = (unsigned long)g_gb * 1024; >> >> This still does not prevent overflows... So what about doing a proper check ? > > This still does not prevent overflows on 32-bits architectures. Because "unsigned long" on 32-bits architectures is 32 bit, so solution 1 is to change the type "unsigned long" to u64, and the diff is as below: diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c index 2f0431e42c49..27a453b3094d 100644 --- a/drivers/block/null_blk/main.c +++ b/drivers/block/null_blk/main.c @@ -762,7 +762,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 */ I just built it and did not make tests. Zhu Yanjun > >> >>> dev->completion_nsec = g_completion_nsec; >>> dev->submit_queues = g_submit_queues; >>> dev->prev_submit_queues = g_submit_queues; >> >
在 2024/09/18 0:29, Zhu Yanjun 写道: > 在 2024/9/17 15:24, Damien Le Moal 写道: >> On 2024/09/17 16:21, Damien Le Moal wrote: >>> On 2024/09/17 16:07, Aleksandr Mishin wrote: >>>> In null_alloc_dev() device size is a subject to overflow because 'g_gb' >>>> (which is module parameter, may have any value and is not validated >>>> anywhere) is not cast to a larger data type before performing >>>> arithmetic. >>>> >>>> Cast 'g_gb' to unsigned long to prevent overflow. >>>> >>>> Found by Linux Verification Center (linuxtesting.org) with SVACE. >>>> >>>> Fixes: 2984c8684f96 ("nullb: factor disk parameters") >>>> Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> >>>> --- >>>> drivers/block/null_blk/main.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/block/null_blk/main.c >>>> b/drivers/block/null_blk/main.c >>>> index 2f0431e42c49..5edbf9c0aceb 100644 >>>> --- a/drivers/block/null_blk/main.c >>>> +++ b/drivers/block/null_blk/main.c >>>> @@ -762,7 +762,7 @@ static struct nullb_device *null_alloc_dev(void) >>>> return NULL; >>>> } >>>> - dev->size = g_gb * 1024; >>>> + dev->size = (unsigned long)g_gb * 1024; >>> >>> This still does not prevent overflows... So what about doing a proper >>> check ? >> >> This still does not prevent overflows on 32-bits architectures. > > Because "unsigned long" on 32-bits architectures is 32 bit, so solution > 1 is to change the type "unsigned long" to u64, and the diff is as below: > > diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c > index 2f0431e42c49..27a453b3094d 100644 > --- a/drivers/block/null_blk/main.c > +++ b/drivers/block/null_blk/main.c > @@ -762,7 +762,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 */ There is more, g_gb is GB, dev->size is MB, and dev->size will be used later for inode size in bytes, and bdev size in sectors. The max inode size is LONG_MAX, this is still more than UINT_MAX GB, so it's right that set the device size by module params won't overflow. However, take a look at setting the size through configfs, the max value is ULONG_MAX MB, this will still overflow. Thanks, Kuai > 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 */ > > I just built it and did not make tests. > > Zhu Yanjun > >> >>> >>>> dev->completion_nsec = g_completion_nsec; >>>> dev->submit_queues = g_submit_queues; >>>> dev->prev_submit_queues = g_submit_queues; >>> >> > > > . >
在 2024/9/18 10:07, Yu Kuai 写道: > > > 在 2024/09/18 0:29, Zhu Yanjun 写道: >> 在 2024/9/17 15:24, Damien Le Moal 写道: >>> On 2024/09/17 16:21, Damien Le Moal wrote: >>>> On 2024/09/17 16:07, Aleksandr Mishin wrote: >>>>> In null_alloc_dev() device size is a subject to overflow because >>>>> 'g_gb' >>>>> (which is module parameter, may have any value and is not validated >>>>> anywhere) is not cast to a larger data type before performing >>>>> arithmetic. >>>>> >>>>> Cast 'g_gb' to unsigned long to prevent overflow. >>>>> >>>>> Found by Linux Verification Center (linuxtesting.org) with SVACE. >>>>> >>>>> Fixes: 2984c8684f96 ("nullb: factor disk parameters") >>>>> Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> >>>>> --- >>>>> drivers/block/null_blk/main.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/block/null_blk/main.c >>>>> b/drivers/block/null_blk/main.c >>>>> index 2f0431e42c49..5edbf9c0aceb 100644 >>>>> --- a/drivers/block/null_blk/main.c >>>>> +++ b/drivers/block/null_blk/main.c >>>>> @@ -762,7 +762,7 @@ static struct nullb_device *null_alloc_dev(void) >>>>> return NULL; >>>>> } >>>>> - dev->size = g_gb * 1024; >>>>> + dev->size = (unsigned long)g_gb * 1024; >>>> >>>> This still does not prevent overflows... So what about doing a >>>> proper check ? >>> >>> This still does not prevent overflows on 32-bits architectures. >> >> Because "unsigned long" on 32-bits architectures is 32 bit, so >> solution 1 is to change the type "unsigned long" to u64, and the diff >> is as below: >> >> diff --git a/drivers/block/null_blk/main.c >> b/drivers/block/null_blk/main.c >> index 2f0431e42c49..27a453b3094d 100644 >> --- a/drivers/block/null_blk/main.c >> +++ b/drivers/block/null_blk/main.c >> @@ -762,7 +762,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 */ > > There is more, g_gb is GB, dev->size is MB, and dev->size will be used > later for inode size in bytes, and bdev size in sectors. > > The max inode size is LONG_MAX, this is still more than UINT_MAX GB, so > it's right that set the device size by module params won't overflow. > > However, take a look at setting the size through configfs, the max value > is ULONG_MAX MB, this will still overflow. Thanks a lot. Add nullb_device_u64_attr_show and nullb_device_u64_attr_store functions. So configfs should work well. Just a draft patch to try to fix this overflow problem. I just built it and did not make tests. diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c index 2f0431e42c49..56ee04277b92 100644 --- a/drivers/block/null_blk/main.c +++ b/drivers/block/null_blk/main.c @@ -289,6 +289,12 @@ 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 +328,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 +458,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 +782,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 */ Zhu Yanjun > > Thanks, > Kuai > >> 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 */ >> >> I just built it and did not make tests. >> >> Zhu Yanjun >> >>> >>>> >>>>> 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/main.c b/drivers/block/null_blk/main.c index 2f0431e42c49..5edbf9c0aceb 100644 --- a/drivers/block/null_blk/main.c +++ b/drivers/block/null_blk/main.c @@ -762,7 +762,7 @@ static struct nullb_device *null_alloc_dev(void) return NULL; } - dev->size = g_gb * 1024; + dev->size = (unsigned long)g_gb * 1024; dev->completion_nsec = g_completion_nsec; dev->submit_queues = g_submit_queues; dev->prev_submit_queues = g_submit_queues;
In null_alloc_dev() device size is a subject to overflow because 'g_gb' (which is module parameter, may have any value and is not validated anywhere) is not cast to a larger data type before performing arithmetic. Cast 'g_gb' to unsigned long to prevent overflow. Found by Linux Verification Center (linuxtesting.org) with SVACE. Fixes: 2984c8684f96 ("nullb: factor disk parameters") Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> --- drivers/block/null_blk/main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)