Re: [PATCH] ocfs2: fix UBSAN warning in ocfs2_verify_volume()

[Joseph Qi <joseph.qi@linux.alibaba.com>]

On 11/5/24 10:05 PM, Dmitry Antipov wrote:
> Syzbot has reported the following splat triggered by UBSAN:
> 
> UBSAN: shift-out-of-bounds in fs/ocfs2/super.c:2336:10
> shift exponent 32768 is too large for 32-bit type 'int'
> CPU: 2 UID: 0 PID: 5255 Comm: repro Not tainted 6.12.0-rc4-syzkaller-00047-gc2ee9f594da8 #0
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-3.fc41 04/01/2014
> Call Trace:
>  <TASK>
>  dump_stack_lvl+0x241/0x360
>  ? __pfx_dump_stack_lvl+0x10/0x10
>  ? __pfx__printk+0x10/0x10
>  ? __asan_memset+0x23/0x50
>  ? lockdep_init_map_type+0xa1/0x910
>  __ubsan_handle_shift_out_of_bounds+0x3c8/0x420
>  ocfs2_fill_super+0xf9c/0x5750
>  ? __pfx_ocfs2_fill_super+0x10/0x10
>  ? __pfx_validate_chain+0x10/0x10
>  ? __pfx_validate_chain+0x10/0x10
>  ? validate_chain+0x11e/0x5920
>  ? __lock_acquire+0x1384/0x2050
>  ? __pfx_validate_chain+0x10/0x10
>  ? string+0x26a/0x2b0
>  ? widen_string+0x3a/0x310
>  ? string+0x26a/0x2b0
>  ? bdev_name+0x2b1/0x3c0
>  ? pointer+0x703/0x1210
>  ? __pfx_pointer+0x10/0x10
>  ? __pfx_format_decode+0x10/0x10
>  ? __lock_acquire+0x1384/0x2050
>  ? vsnprintf+0x1ccd/0x1da0
>  ? snprintf+0xda/0x120
>  ? __pfx_lock_release+0x10/0x10
>  ? do_raw_spin_lock+0x14f/0x370
>  ? __pfx_snprintf+0x10/0x10
>  ? set_blocksize+0x1f9/0x360
>  ? sb_set_blocksize+0x98/0xf0
>  ? setup_bdev_super+0x4e6/0x5d0
>  mount_bdev+0x20c/0x2d0
>  ? __pfx_ocfs2_fill_super+0x10/0x10
>  ? __pfx_mount_bdev+0x10/0x10
>  ? vfs_parse_fs_string+0x190/0x230
>  ? __pfx_vfs_parse_fs_string+0x10/0x10
>  legacy_get_tree+0xf0/0x190
>  ? __pfx_ocfs2_mount+0x10/0x10
>  vfs_get_tree+0x92/0x2b0
>  do_new_mount+0x2be/0xb40
>  ? __pfx_do_new_mount+0x10/0x10
>  __se_sys_mount+0x2d6/0x3c0
>  ? __pfx___se_sys_mount+0x10/0x10
>  ? do_syscall_64+0x100/0x230
>  ? __x64_sys_mount+0x20/0xc0
>  do_syscall_64+0xf3/0x230
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> RIP: 0033:0x7f37cae96fda
> Code: 48 8b 0d 51 ce 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 1e ce 0c 00 f7 d8 64 89 01 48
> RSP: 002b:00007fff6c1aa228 EFLAGS: 00000206 ORIG_RAX: 00000000000000a5
> RAX: ffffffffffffffda RBX: 00007fff6c1aa240 RCX: 00007f37cae96fda
> RDX: 00000000200002c0 RSI: 0000000020000040 RDI: 00007fff6c1aa240
> RBP: 0000000000000004 R08: 00007fff6c1aa280 R09: 0000000000000000
> R10: 00000000000008c0 R11: 0000000000000206 R12: 00000000000008c0
> R13: 00007fff6c1aa280 R14: 0000000000000003 R15: 0000000001000000
>  </TASK>
> 
> For a really damaged superblock, the value of 'i_super.s_blocksize_bits'
> may exceed the maximum possible shift for an underlying 'int'. So add an
> extra check whether the aforementioned field represents the valid block
> size, which is 512 bytes, 1K, 2K, or 4K.
> 
> Reported-by: syzbot+56f7cd1abe4b8e475180@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=56f7cd1abe4b8e475180
> Fixes: ccd979bdbce9 ("[PATCH] OCFS2: The Second Oracle Cluster Filesystem")
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
>  fs/ocfs2/super.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
> index 3d404624bb96..9852067570e3 100644
> --- a/fs/ocfs2/super.c
> +++ b/fs/ocfs2/super.c
> @@ -2319,6 +2319,7 @@ static int ocfs2_verify_volume(struct ocfs2_dinode *di,
>  			       struct ocfs2_blockcheck_stats *stats)
>  {
>  	int status = -EAGAIN;
> +	u32 blkszbit;
>  
>

Better to rename it to 'blksz_bits'.

  	if (memcmp(di->i_signature, OCFS2_SUPER_BLOCK_SIGNATURE,
>  		   strlen(OCFS2_SUPER_BLOCK_SIGNATURE)) == 0) {
> @@ -2333,11 +2334,15 @@ static int ocfs2_verify_volume(struct ocfs2_dinode *di,
>  				goto out;
>  		}
>  		status = -EINVAL;
> -		if ((1 << le32_to_cpu(di->id2.i_super.s_blocksize_bits)) != blksz) {
> +		/* Acceptable block sizes are 512 bytes, 1K, 2K and 4K. */
> +		blkszbit = le32_to_cpu(di->id2.i_super.s_blocksize_bits);
> +		if (blkszbit < 9 || blkszbit > 12) {
>  			mlog(ML_ERROR, "found superblock with incorrect block "
> -			     "size: found %u, should be %u\n",
> -			     1 << le32_to_cpu(di->id2.i_super.s_blocksize_bits),
> -			       blksz);
> +			     "size bit: found %u, should be 9, 10, 11, or 12\n",

s/size bit/size bits

Other looks good to me.

Thanks,
Joseph

> +			     blkszbit);
> +		} else if ((1 << le32_to_cpu(blkszbit)) != blksz) {
> +			mlog(ML_ERROR, "found superblock with incorrect block "
> +			     "size: found %u, should be %u\n", 1 << blkszbit, blksz);
>  		} else if (le16_to_cpu(di->id2.i_super.s_major_rev_level) !=
>  			   OCFS2_MAJOR_REV_LEVEL ||
>  			   le16_to_cpu(di->id2.i_super.s_minor_rev_level) !=