nullb: Adjust device size calculation in null_alloc_dev()

[PATCH] nullb: Adjust device size calculation in null_alloc_dev()

Posted by Aleksandr Mishin 1 year, 4 months ago

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;
 	dev->completion_nsec = g_completion_nsec;
 	dev->submit_queues = g_submit_queues;
 	dev->prev_submit_queues = g_submit_queues;
-- 
2.30.2

Re: [PATCH] nullb: Adjust device size calculation in null_alloc_dev()

Posted by Damien Le Moal 1 year, 4 months ago

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;

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] nullb: Adjust device size calculation in null_alloc_dev()

Posted by Damien Le Moal 1 year, 4 months ago

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;
> 

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] nullb: Adjust device size calculation in null_alloc_dev()

Posted by Zhu Yanjun 1 year, 4 months ago

在 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;
>>
>

Re: [PATCH] nullb: Adjust device size calculation in null_alloc_dev()

Posted by Yu Kuai 1 year, 4 months ago


在 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;
>>>
>>
> 
> 
> .
>

Re: [PATCH] nullb: Adjust device size calculation in null_alloc_dev()

Posted by Zhu Yanjun 1 year, 4 months ago

在 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;
>>>>
>>>
>>
>>
>> .
>>
>
-- 
Best Regards,
Yanjun.Zhu

Re: [PATCH] nullb: Adjust device size calculation in null_alloc_dev()

Posted by Zhu Yanjun 1 year, 4 months ago

在 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;

-- 
Best Regards,
Yanjun.Zhu

Re: [PATCH] nullb: Adjust device size calculation in null_alloc_dev()

Posted by Zhu Yanjun 1 year, 4 months ago

在 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;

-- 
Best Regards,
Yanjun.Zhu