[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20221114120732.vephphzzpdkv2zam@quack3>
Date: Mon, 14 Nov 2022 13:07:32 +0100
From: Jan Kara <jack@...e.cz>
To: "liaochang (A)" <liaochang1@...wei.com>
Cc: Jan Kara <jack@...e.cz>, willy@...radead.org,
damien.lemoal@...nsource.wdc.com, akpm@...ux-foundation.org,
qhjin.dev@...il.com, songmuchun@...edance.com,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fs: Fix UBSAN detected shift-out-bounds error for bad
superblock
On Mon 14-11-22 19:59:03, liaochang (A) wrote:
>
>
> 在 2022/11/14 19:04, Jan Kara 写道:
> > On Mon 14-11-22 10:49:57, Liao Chang wrote:
> >> UBSAN: shift-out-of-bounds in fs/minix/bitmap.c:103:3
> >> shift exponent 8192 is too large for 32-bit type 'unsigned int'
> >> CPU: 1 PID: 32273 Comm: syz-executor.0 Tainted: G W
> >> 6.1.0-rc4-dirty #11
> >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> >> BIOS rel-1.15.0-0-g2dd4b9b3f840-prebuilt.qemu.org 04/01/2014
> >> Call Trace:
> >> <TASK>
> >> dump_stack_lvl+0xcd/0x134
> >> ubsan_epilogue+0xb/0x50
> >> __ubsan_handle_shift_out_of_bounds.cold+0xb1/0x18d
> >> minix_count_free_blocks.cold+0x16/0x1b
> >> minix_statfs+0x22a/0x490
> >> statfs_by_dentry+0x133/0x210
> >> user_statfs+0xa9/0x160
> >> __do_sys_statfs+0x7a/0xf0
> >> do_syscall_64+0x35/0x80
> >> entry_SYSCALL_64_after_hwframe+0x63/0xcd
> >>
> >> The superblock stores on disk contains the size of a data zone, which is
> >> too large to used as the shift when kernel try to calculate the total
> >> size of zones, so it needs to check the superblock when kernel mounts
> >> MINIX-FS.
> >>
> >> Signed-off-by: Liao Chang <liaochang1@...wei.com>
> >
> > Thanks for the patch. Just one nit:
> >
> >> diff --git a/fs/minix/inode.c b/fs/minix/inode.c
> >> index da8bdd1712a7..f1d1c2312817 100644
> >> --- a/fs/minix/inode.c
> >> +++ b/fs/minix/inode.c
> >> @@ -166,6 +166,12 @@ static bool minix_check_superblock(struct super_block *sb)
> >> sb->s_maxbytes > (7 + 512 + 512*512) * BLOCK_SIZE)
> >> return false;
> >>
> >> + /* the total size of zones must no exceed the limitation of U32_MAX. */
> >> + if (sbi->s_log_zone_size && (sbi->s_nzones - sbi->s_firstdatazone) &&
> >> + (__builtin_clzl((__u32)(sbi->s_nzones - sbi->s_firstdatazone)) <=
> >
> > Why this strange __builtin_clzl() function? We have a ffs() function in
> > the kernel for this :)
>
> Great suggestion, i should use a compiler neutral API to caclulate leading zero count,
> what about count_leading_zeros()?
Yeah, that would work as well.
Honza
--
Jan Kara <jack@...e.com>
SUSE Labs, CR
Powered by blists - more mailing lists