| Message ID | f81aaa2b-16c4-6e20-8a13-33f0a7d319d1@i-love.sakura.ne.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: use "unsigned long" for blk_validate_block_size() | expand |
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.
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.
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().
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;
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(-)