lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <e5807f3c-3173-44e6-b222-fc4679be4680@linux.dev>
Date: Wed, 18 Sep 2024 10:57:10 +0800
From: Zhu Yanjun <yanjun.zhu@...ux.dev>
To: Yu Kuai <yukuai1@...weicloud.com>, Damien Le Moal <dlemoal@...nel.org>,
 Aleksandr Mishin <amishin@...rgos.ru>, Shaohua Li <shli@...com>
Cc: Jens Axboe <axboe@...nel.dk>, Hannes Reinecke <hare@...e.de>,
 Johannes Thumshirn <johannes.thumshirn@....com>,
 Chaitanya Kulkarni <kch@...dia.com>,
 Chengming Zhou <zhouchengming@...edance.com>,
 John Garry <john.g.garry@...cle.com>,
 Shin'ichiro Kawasaki <shinichiro.kawasaki@....com>,
 linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
 lvc-project@...uxtesting.org, "yukuai (C)" <yukuai3@...wei.com>,
 "yangerkun@...wei.com" <yangerkun@...wei.com>
Subject: Re: [PATCH] nullb: Adjust device size calculation in null_alloc_dev()


在 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@...rgos.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


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ