UBSAN: shift-out-of-bounds in exfat_fill_super

UBSAN: shift-out-of-bounds in exfat_fill_super

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    9791581c Merge tag 'for-5.11-rc4-tag' of git://git.kernel...
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=10d94027500000
kernel config:  https://syzkaller.appspot.com/x/.config?x=c5f3af3f625fb042
dashboard link: https://syzkaller.appspot.com/bug?extid=da4fe66aaadd3c2e2d1c
compiler:       clang version 11.0.1
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=11ad6f00d00000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=13ffb69f500000

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+da4fe66aaadd3c2e2d1c@syzkaller.appspotmail.com

loop0: detected capacity change from 16383 to 0
================================================================================
UBSAN: shift-out-of-bounds in fs/exfat/super.c:471:28
shift exponent 4294967294 is too large for 32-bit type 'int'
CPU: 0 PID: 8443 Comm: syz-executor876 Not tainted 5.11.0-rc4-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:79 [inline]
 dump_stack+0x183/0x22e lib/dump_stack.c:120
 ubsan_epilogue lib/ubsan.c:148 [inline]
 __ubsan_handle_shift_out_of_bounds+0x432/0x4d0 lib/ubsan.c:395
 exfat_read_boot_sector fs/exfat/super.c:471 [inline]
 __exfat_fill_super fs/exfat/super.c:556 [inline]
 exfat_fill_super+0x2acb/0x2d00 fs/exfat/super.c:624
 get_tree_bdev+0x406/0x630 fs/super.c:1291
 vfs_get_tree+0x86/0x270 fs/super.c:1496
 do_new_mount fs/namespace.c:2881 [inline]
 path_mount+0x1937/0x2c50 fs/namespace.c:3211
 do_mount fs/namespace.c:3224 [inline]
 __do_sys_mount fs/namespace.c:3432 [inline]
 __se_sys_mount+0x2f9/0x3b0 fs/namespace.c:3409
 do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
 entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x446f8a
Code: b8 08 00 00 00 0f 05 48 3d 01 f0 ff ff 0f 83 fd ad fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 0f 83 da ad fb ff c3 66 0f 1f 84 00 00 00 00 00
RSP: 002b:00007fff97c1b978 EFLAGS: 00000297 ORIG_RAX: 00000000000000a5
RAX: ffffffffffffffda RBX: 00007fff97c1b9d0 RCX: 0000000000446f8a
RDX: 0000000020000000 RSI: 0000000020000340 RDI: 00007fff97c1b990
RBP: 00007fff97c1b990 R08: 00007fff97c1b9d0 R09: 00007fff00000015
R10: 0000000000000000 R11: 0000000000000297 R12: 000000000000000c
R13: 0000000000000004 R14: 0000000000000003 R15: 0000000000000003
================================================================================


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches

Re: UBSAN: shift-out-of-bounds in exfat_fill_super

[Matthew Wilcox]

On Mon, Jan 25, 2021 at 09:33:14AM -0800, syzbot wrote:
> UBSAN: shift-out-of-bounds in fs/exfat/super.c:471:28
> shift exponent 4294967294 is too large for 32-bit type 'int'

This is an integer underflow:

        sbi->dentries_per_clu = 1 <<
                (sbi->cluster_size_bits - DENTRY_SIZE_BITS);

I think the problem is that there is no validation of sect_per_clus_bits.
We should check it is at least DENTRY_SIZE_BITS and probably that it's
less than ... 16?  64?  I don't know what legitimate values are in this
field, but I would imagine that 255 is completely unacceptable.

Re: UBSAN: shift-out-of-bounds in exfat_fill_super

[Randy Dunlap]

On 1/25/21 10:39 AM, Matthew Wilcox wrote:
> On Mon, Jan 25, 2021 at 09:33:14AM -0800, syzbot wrote:
>> UBSAN: shift-out-of-bounds in fs/exfat/super.c:471:28
>> shift exponent 4294967294 is too large for 32-bit type 'int'
> 
> This is an integer underflow:
> 
>         sbi->dentries_per_clu = 1 <<
>                 (sbi->cluster_size_bits - DENTRY_SIZE_BITS);
> 
> I think the problem is that there is no validation of sect_per_clus_bits.
> We should check it is at least DENTRY_SIZE_BITS and probably that it's
> less than ... 16?  64?  I don't know what legitimate values are in this
> field, but I would imagine that 255 is completely unacceptable.

Ack all of that. The syzbot boot_sector has sect_per_clus_bits == 3
and sect_size_bits == 0, so sbi->cluster_size_bits is 3, then
UBSAN goes bang on:

	sbi->dentries_per_clu = 1 <<
		(sbi->cluster_size_bits - DENTRY_SIZE_BITS); // 3 - 5


There is also an unprotected shift at line 480:

	if (sbi->num_FAT_sectors << p_boot->sect_size_bits <
	    sbi->num_clusters * 4) {

that should be protected IMO.


-- 
~Randy

RE: UBSAN: shift-out-of-bounds in exfat_fill_super

[Namjae Jeon]

> On Mon, Jan 25, 2021 at 09:33:14AM -0800, syzbot wrote:
> > UBSAN: shift-out-of-bounds in fs/exfat/super.c:471:28 shift exponent
> > 4294967294 is too large for 32-bit type 'int'
> 
> This is an integer underflow:
> 
>         sbi->dentries_per_clu = 1 <<
>                 (sbi->cluster_size_bits - DENTRY_SIZE_BITS);
> 
> I think the problem is that there is no validation of sect_per_clus_bits.
> We should check it is at least DENTRY_SIZE_BITS and probably that it's less than ... 16?  64?  I don't
> know what legitimate values are in this field, but I would imagine that 255 is completely unacceptable.
exfat specification describe sect_per_clus_bits field of boot sector could be at most 32 and
at least 0. And sect_size_bits can also affect this calculation, It also needs validation.
I will fix it.
Thanks!

RE: UBSAN: shift-out-of-bounds in exfat_fill_super

[Namjae Jeon]

> > On Mon, Jan 25, 2021 at 09:33:14AM -0800, syzbot wrote:
> > > UBSAN: shift-out-of-bounds in fs/exfat/super.c:471:28 shift exponent
> > > 4294967294 is too large for 32-bit type 'int'
> >
> > This is an integer underflow:
> >
> >         sbi->dentries_per_clu = 1 <<
> >                 (sbi->cluster_size_bits - DENTRY_SIZE_BITS);
> >
> > I think the problem is that there is no validation of sect_per_clus_bits.
> > We should check it is at least DENTRY_SIZE_BITS and probably that it's
> > less than ... 16?  64?  I don't know what legitimate values are in this field, but I would imagine
> that 255 is completely unacceptable.
> exfat specification describe sect_per_clus_bits field of boot sector could be at most 32 and at least
                                                                                                 typo ^^16
> 0. And sect_size_bits can also affect this calculation, It also needs validation.
> I will fix it.
> Thanks!

RE: UBSAN: shift-out-of-bounds in exfat_fill_super

[Namjae Jeon]

> On 1/25/21 10:39 AM, Matthew Wilcox wrote:
> > On Mon, Jan 25, 2021 at 09:33:14AM -0800, syzbot wrote:
> >> UBSAN: shift-out-of-bounds in fs/exfat/super.c:471:28 shift exponent
> >> 4294967294 is too large for 32-bit type 'int'
> >
> > This is an integer underflow:
> >
> >         sbi->dentries_per_clu = 1 <<
> >                 (sbi->cluster_size_bits - DENTRY_SIZE_BITS);
> >
> > I think the problem is that there is no validation of sect_per_clus_bits.
> > We should check it is at least DENTRY_SIZE_BITS and probably that it's
> > less than ... 16?  64?  I don't know what legitimate values are in
> > this field, but I would imagine that 255 is completely unacceptable.
> 
> Ack all of that. The syzbot boot_sector has sect_per_clus_bits == 3 and sect_size_bits == 0, so sbi-
> >cluster_size_bits is 3, then UBSAN goes bang on:
> 
> 	sbi->dentries_per_clu = 1 <<
> 		(sbi->cluster_size_bits - DENTRY_SIZE_BITS); // 3 - 5
> 
> 
> There is also an unprotected shift at line 480:
> 
> 	if (sbi->num_FAT_sectors << p_boot->sect_size_bits <
> 	    sbi->num_clusters * 4) {
> 
> that should be protected IMO.
Right. I will also add validation for fat_length as well as sect_size_bits before this.

Thanks!
> 
> 
> --
> ~Randy