[PATCH] block: use "unsigned long" for blk_validate_block_size()

[PATCH] block: use "unsigned long" for blk_validate_block_size()

[Tetsuo Handa]

Use of "unsigned short" for loop_validate_block_size() is wrong [1], and
commit af3c570fb0df422b ("loop: Use blk_validate_block_size() to validate
block size") changed to use "unsigned int".

However, since lo_simple_ioctl(LOOP_SET_BLOCK_SIZE) passes "unsigned long
arg" to loop_set_block_size(), blk_validate_block_size() can't validate
the upper 32bits on 64bits environment. A block size like 0x100000200
should be rejected.

Link: https://lkml.kernel.org/r/20210927094327.644665-1-arnd@kernel.org [1]
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 include/linux/blkdev.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index bd4370baccca..e13e41f7fad2 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -44,7 +44,7 @@ struct blk_crypto_profile;
  */
 #define BLKCG_MAX_POLS		6
 
-static inline int blk_validate_block_size(unsigned int bsize)
+static inline int blk_validate_block_size(unsigned long bsize)
 {
 	if (bsize < 512 || bsize > PAGE_SIZE || !is_power_of_2(bsize))
 		return -EINVAL;
-- 
2.32.0

Re: [PATCH] block: use "unsigned long" for blk_validate_block_size()

[Jens Axboe]

On 12/17/21 4:38 AM, Tetsuo Handa wrote:
> Use of "unsigned short" for loop_validate_block_size() is wrong [1], and
> commit af3c570fb0df422b ("loop: Use blk_validate_block_size() to validate
> block size") changed to use "unsigned int".
> 
> However, since lo_simple_ioctl(LOOP_SET_BLOCK_SIZE) passes "unsigned long
> arg" to loop_set_block_size(), blk_validate_block_size() can't validate
> the upper 32bits on 64bits environment. A block size like 0x100000200
> should be rejected.

Wouldn't it make more sense to validate that part on the loop side? A
block size > 32-bit doesn't make any sense.

-- 
Jens Axboe

Re: [PATCH] block: use "unsigned long" for blk_validate_block_size()

[Tetsuo Handa]

On 2021/12/18 1:25, Jens Axboe wrote:
> On 12/17/21 4:38 AM, Tetsuo Handa wrote:
>> Use of "unsigned short" for loop_validate_block_size() is wrong [1], and
>> commit af3c570fb0df422b ("loop: Use blk_validate_block_size() to validate
>> block size") changed to use "unsigned int".
>>
>> However, since lo_simple_ioctl(LOOP_SET_BLOCK_SIZE) passes "unsigned long
>> arg" to loop_set_block_size(), blk_validate_block_size() can't validate
>> the upper 32bits on 64bits environment. A block size like 0x100000200
>> should be rejected.
> 
> Wouldn't it make more sense to validate that part on the loop side? A
> block size > 32-bit doesn't make any sense.
> 

I think doing below is embarrassing, for there is blk_validate_block_size() which is
meant for validating the arg. Although use of "unsigned long" for blk_validate_block_size()
might cause small bloating on 64 bits kernels, I think 64 bits kernels would not care.

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index c3a36cfaa855..98871d7b601d 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1474,6 +1474,10 @@ static int loop_set_block_size(struct loop_device *lo, unsigned long arg)
 	err = blk_validate_block_size(arg);
 	if (err)
 		return err;
+#if BITS_PER_LONG == 64
+	if (arg > UINT_MAX)
+		return -EINVAL;
+#endif
 
 	if (lo->lo_queue->limits.logical_block_size == arg)
 		return 0;

And reviving loop_validate_block_size() in order to use "unsigned long" does not make sense
for 32bits kernels.

Re: [PATCH] block: use "unsigned long" for blk_validate_block_size()

[Tetsuo Handa]

On 2021/12/18 3:09, Tetsuo Handa wrote:
> On 2021/12/18 1:25, Jens Axboe wrote:
>> On 12/17/21 4:38 AM, Tetsuo Handa wrote:
>>> Use of "unsigned short" for loop_validate_block_size() is wrong [1], and
>>> commit af3c570fb0df422b ("loop: Use blk_validate_block_size() to validate
>>> block size") changed to use "unsigned int".
>>>
>>> However, since lo_simple_ioctl(LOOP_SET_BLOCK_SIZE) passes "unsigned long
>>> arg" to loop_set_block_size(), blk_validate_block_size() can't validate
>>> the upper 32bits on 64bits environment. A block size like 0x100000200
>>> should be rejected.
>>
>> Wouldn't it make more sense to validate that part on the loop side? A
>> block size > 32-bit doesn't make any sense.
>>
> 
> I think doing below is embarrassing, for there is blk_validate_block_size() which is
> meant for validating the arg. Although use of "unsigned long" for blk_validate_block_size()
> might cause small bloating on 64 bits kernels, I think 64 bits kernels would not care.
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index c3a36cfaa855..98871d7b601d 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -1474,6 +1474,10 @@ static int loop_set_block_size(struct loop_device *lo, unsigned long arg)
>  	err = blk_validate_block_size(arg);
>  	if (err)
>  		return err;
> +#if BITS_PER_LONG == 64
> +	if (arg > UINT_MAX)
> +		return -EINVAL;
> +#endif
>  
>  	if (lo->lo_queue->limits.logical_block_size == arg)
>  		return 0;
> 
> And reviving loop_validate_block_size() in order to use "unsigned long" does not make sense
> for 32bits kernels.

Well, this problem is not specific to the loop module.
nbd_set_size(struct nbd_device *nbd, loff_t bytesize, loff_t blksize) calls
blk_validate_block_size(blksize), and blksize can be > UINT_MAX, for
ioctl(NBD_SET_BLKSIZE) passes "unsigned long arg" to nbd_set_size().

But size bloating cannot be a problem because there are only 4 callers.

    drivers/block/loop.c
        line 996
        line 1474 
    drivers/block/nbd.c, line 325
    drivers/block/virtio_blk.c, line 878 

I think we can (and should) use "unsigned long" for blk_validate_block_size().

[PATCH v2] block: use "unsigned long" for blk_validate_block_size().

[Tetsuo Handa]

Since lo_simple_ioctl(LOOP_SET_BLOCK_SIZE) and ioctl(NBD_SET_BLKSIZE) pass
user-controlled "unsigned long arg" to blk_validate_block_size(),
"unsigned long" should be used for validation.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
Changes in v2:
  Rewrite description.

 include/linux/blkdev.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index bd4370baccca..e13e41f7fad2 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -44,7 +44,7 @@ struct blk_crypto_profile;
  */
 #define BLKCG_MAX_POLS		6
 
-static inline int blk_validate_block_size(unsigned int bsize)
+static inline int blk_validate_block_size(unsigned long bsize)
 {
 	if (bsize < 512 || bsize > PAGE_SIZE || !is_power_of_2(bsize))
 		return -EINVAL;
-- 
2.32.0

Re: [PATCH v2] block: use "unsigned long" for blk_validate_block_size().

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2] block: use "unsigned long" for blk_validate_block_size().

[Jens Axboe]

On Sat, 18 Dec 2021 18:41:56 +0900, Tetsuo Handa wrote:
> Since lo_simple_ioctl(LOOP_SET_BLOCK_SIZE) and ioctl(NBD_SET_BLKSIZE) pass
> user-controlled "unsigned long arg" to blk_validate_block_size(),
> "unsigned long" should be used for validation.
> 
> 

Applied, thanks!

[1/1] block: use "unsigned long" for blk_validate_block_size().
      commit: 37ae5a0f5287a52cf51242e76ccf198d02ffe495

Best regards,
-- 
Jens Axboe