On 2021/12/18 1:25, Jens Axboe wrote:

quoted

On 12/17/21 4:38 AM, Tetsuo Handa wrote:

quoted

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.