[v1] fs: Fix UBSAN detected shift-out-bounds error for bad superblock

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

Posted by Liao Chang 3 years, 5 months ago

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@huawei.com>
---
 fs/minix/inode.c | 6 ++++++
 1 file changed, 6 insertions(+)

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)) <=
+	     sbi->s_log_zone_size))
+		return false;
+
 	return true;
 }
 
-- 
2.17.1

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

Posted by Randy Dunlap 3 years, 4 months ago


On 11/13/22 18:49, 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@huawei.com>
> ---
>  fs/minix/inode.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> 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. */

	                           must not exceed

> +	if (sbi->s_log_zone_size && (sbi->s_nzones - sbi->s_firstdatazone) &&
> +	    (__builtin_clzl((__u32)(sbi->s_nzones - sbi->s_firstdatazone)) <=
> +	     sbi->s_log_zone_size))
> +		return false;
> +
>  	return true;
>  }
>  

-- 
~Randy

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

Posted by liaochang (A) 3 years, 4 months ago


在 2022/11/15 5:09, Randy Dunlap 写道:
> 
> 
> On 11/13/22 18:49, 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@huawei.com>
>> ---
>>  fs/minix/inode.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> 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. */
> 
> 	                           must not exceed

Thanks, i will correct it in next revision.

> 
>> +	if (sbi->s_log_zone_size && (sbi->s_nzones - sbi->s_firstdatazone) &&
>> +	    (__builtin_clzl((__u32)(sbi->s_nzones - sbi->s_firstdatazone)) <=
>> +	     sbi->s_log_zone_size))
>> +		return false;
>> +
>>  	return true;
>>  }
>>  
> 

-- 
BR,
Liao, Chang

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

Posted by Jan Kara 3 years, 5 months ago

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@huawei.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@suse.com>
SUSE Labs, CR

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

Posted by liaochang (A) 3 years, 5 months ago


在 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@huawei.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()?

Thanks.

> 
> 								Honza
> 
>> +	     sbi->s_log_zone_size))
>> +		return false;
>> +
>>  	return true;
>>  }
>>  
>> -- 
>> 2.17.1
>>

-- 
BR,
Liao, Chang

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

Posted by Jan Kara 3 years, 5 months ago

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@huawei.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@suse.com>
SUSE Labs, CR