linux-kernel - Re: [PATCH] fs: Fix UBSAN detected shift-out-bounds error for bad superblock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20221114110459.x7yb6rhgwpi6kyjj@quack3>
Date:   Mon, 14 Nov 2022 12:04:59 +0100
From:   Jan Kara <jack@...e.cz>
To:     Liao Chang <liaochang1@...wei.com>
Cc:     willy@...radead.org, damien.lemoal@...nsource.wdc.com,
        akpm@...ux-foundation.org, jack@...e.cz, 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 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 :)

								Honza

> +	     sbi->s_log_zone_size))
> +		return false;
> +
>  	return true;
>  }
>  
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR