All of lore.kernel.org
 help / color / mirror / Atom feed
* UBSAN: shift-out-of-bounds in ext4_fill_super
@ 2020-12-09  7:33 syzbot
  2020-12-10  2:36 ` Theodore Y. Ts'o
  0 siblings, 1 reply; 10+ messages in thread
From: syzbot @ 2020-12-09  7:33 UTC (permalink / raw)
  To: adilger.kernel, clang-built-linux, linux-ext4, linux-kernel,
	natechancellor, ndesaulniers, syzkaller-bugs, tytso

Hello,

syzbot found the following issue on:

HEAD commit:    15ac8fdb Add linux-next specific files for 20201207
git tree:       linux-next
console output: https://syzkaller.appspot.com/x/log.txt?x=1125c923500000
kernel config:  https://syzkaller.appspot.com/x/.config?x=3696b8138207d24d
dashboard link: https://syzkaller.appspot.com/bug?extid=345b75652b1d24227443
compiler:       gcc (GCC) 10.1.0-syz 20200507
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=151bf86b500000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=139212cb500000

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

loop0: detected capacity change from 4 to 0
================================================================================
UBSAN: shift-out-of-bounds in fs/ext4/super.c:4190:25
shift exponent 589825 is too large for 32-bit type 'int'
CPU: 1 PID: 8498 Comm: syz-executor023 Not tainted 5.10.0-rc6-next-20201207-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+0x107/0x163 lib/dump_stack.c:120
 ubsan_epilogue+0xb/0x5a lib/ubsan.c:148
 __ubsan_handle_shift_out_of_bounds.cold+0xb1/0x181 lib/ubsan.c:395
 ext4_fill_super.cold+0x154/0x3ce fs/ext4/super.c:4190
 mount_bdev+0x34d/0x410 fs/super.c:1366
 legacy_get_tree+0x105/0x220 fs/fs_context.c:592
 vfs_get_tree+0x89/0x2f0 fs/super.c:1496
 do_new_mount fs/namespace.c:2896 [inline]
 path_mount+0x12ae/0x1e70 fs/namespace.c:3227
 do_mount fs/namespace.c:3240 [inline]
 __do_sys_mount fs/namespace.c:3448 [inline]
 __se_sys_mount fs/namespace.c:3425 [inline]
 __x64_sys_mount+0x27f/0x300 fs/namespace.c:3425
 do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
 entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x446d6a
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:00007ffc2d215018 EFLAGS: 00000206 ORIG_RAX: 00000000000000a5
RAX: ffffffffffffffda RBX: 00007ffc2d215070 RCX: 0000000000446d6a
RDX: 0000000020000000 RSI: 0000000020000100 RDI: 00007ffc2d215030
RBP: 00007ffc2d215030 R08: 00007ffc2d215070 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000206 R12: 0000000000000001
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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: UBSAN: shift-out-of-bounds in ext4_fill_super
  2020-12-09  7:33 UBSAN: shift-out-of-bounds in ext4_fill_super syzbot
@ 2020-12-10  2:36 ` Theodore Y. Ts'o
  2020-12-10  3:50   ` syzbot
  0 siblings, 1 reply; 10+ messages in thread
From: Theodore Y. Ts'o @ 2020-12-10  2:36 UTC (permalink / raw)
  To: syzbot
  Cc: adilger.kernel, clang-built-linux, linux-ext4, linux-kernel,
	natechancellor, ndesaulniers, syzkaller-bugs

On Tue, Dec 08, 2020 at 11:33:11PM -0800, syzbot wrote:
> Hello,
> 
> syzbot found the following issue on:
> 
> HEAD commit:    15ac8fdb Add linux-next specific files for 20201207
> git tree:       linux-next
> console output: https://syzkaller.appspot.com/x/log.txt?x=1125c923500000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=3696b8138207d24d
> dashboard link: https://syzkaller.appspot.com/bug?extid=345b75652b1d24227443
> compiler:       gcc (GCC) 10.1.0-syz 20200507
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=151bf86b500000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=139212cb500000

#syz test git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git e360ba58d067a30a4e3e7d55ebdd919885a058d6

From 3d3bc303a8a8f7123cf486f49fa9060116fa1465 Mon Sep 17 00:00:00 2001
From: Theodore Ts'o <tytso@mit.edu>
Date: Wed, 9 Dec 2020 15:59:11 -0500
Subject: [PATCH] ext4: check for invalid block size early when mounting a file
 system

Check for valid block size directly by validating s_log_block_size; we
were doing this in two places.  First, by calculating blocksize via
BLOCK_SIZE << s_log_block_size, and then checking that the blocksize
was valid.  And then secondly, by checking s_log_block_size directly.

The first check is not reliable, and can trigger an UBSAN warning if
s_log_block_size on a maliciously corrupted superblock is greater than
22.  This is harmless, since the second test will correctly reject the
maliciously fuzzed file system, but to make syzbot shut up, and
because the two checks are duplicative in any case, delete the
blocksize check, and move the s_log_block_size earlier in
ext4_fill_super().

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Reported-by: syzbot+345b75652b1d24227443@syzkaller.appspotmail.com
---
 fs/ext4/super.c | 40 ++++++++++++++++------------------------
 1 file changed, 16 insertions(+), 24 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index f86220a8df50..4a16bbf0432c 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4202,18 +4202,25 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	 */
 	sbi->s_li_wait_mult = EXT4_DEF_LI_WAIT_MULT;
 
-	blocksize = BLOCK_SIZE << le32_to_cpu(es->s_log_block_size);
-
-	if (blocksize == PAGE_SIZE)
-		set_opt(sb, DIOREAD_NOLOCK);
-
-	if (blocksize < EXT4_MIN_BLOCK_SIZE ||
-	    blocksize > EXT4_MAX_BLOCK_SIZE) {
+	if (le32_to_cpu(es->s_log_block_size) >
+	    (EXT4_MAX_BLOCK_LOG_SIZE - EXT4_MIN_BLOCK_LOG_SIZE)) {
 		ext4_msg(sb, KERN_ERR,
-		       "Unsupported filesystem blocksize %d (%d log_block_size)",
-			 blocksize, le32_to_cpu(es->s_log_block_size));
+			 "Invalid log block size: %u",
+			 le32_to_cpu(es->s_log_block_size));
 		goto failed_mount;
 	}
+	if (le32_to_cpu(es->s_log_cluster_size) >
+	    (EXT4_MAX_CLUSTER_LOG_SIZE - EXT4_MIN_BLOCK_LOG_SIZE)) {
+		ext4_msg(sb, KERN_ERR,
+			 "Invalid log cluster size: %u",
+			 le32_to_cpu(es->s_log_cluster_size));
+		goto failed_mount;
+	}
+
+	blocksize = EXT4_MIN_BLOCK_SIZE << le32_to_cpu(es->s_log_block_size);
+
+	if (blocksize == PAGE_SIZE)
+		set_opt(sb, DIOREAD_NOLOCK);
 
 	if (le32_to_cpu(es->s_rev_level) == EXT4_GOOD_OLD_REV) {
 		sbi->s_inode_size = EXT4_GOOD_OLD_INODE_SIZE;
@@ -4432,21 +4439,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	if (!ext4_feature_set_ok(sb, (sb_rdonly(sb))))
 		goto failed_mount;
 
-	if (le32_to_cpu(es->s_log_block_size) >
-	    (EXT4_MAX_BLOCK_LOG_SIZE - EXT4_MIN_BLOCK_LOG_SIZE)) {
-		ext4_msg(sb, KERN_ERR,
-			 "Invalid log block size: %u",
-			 le32_to_cpu(es->s_log_block_size));
-		goto failed_mount;
-	}
-	if (le32_to_cpu(es->s_log_cluster_size) >
-	    (EXT4_MAX_CLUSTER_LOG_SIZE - EXT4_MIN_BLOCK_LOG_SIZE)) {
-		ext4_msg(sb, KERN_ERR,
-			 "Invalid log cluster size: %u",
-			 le32_to_cpu(es->s_log_cluster_size));
-		goto failed_mount;
-	}
-
 	if (le16_to_cpu(sbi->s_es->s_reserved_gdt_blocks) > (blocksize / 4)) {
 		ext4_msg(sb, KERN_ERR,
 			 "Number of reserved GDT blocks insanely large: %d",
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: UBSAN: shift-out-of-bounds in ext4_fill_super
  2020-12-10  2:36 ` Theodore Y. Ts'o
@ 2020-12-10  3:50   ` syzbot
  2020-12-10  8:09     ` Dmitry Vyukov
  0 siblings, 1 reply; 10+ messages in thread
From: syzbot @ 2020-12-10  3:50 UTC (permalink / raw)
  To: adilger.kernel, clang-built-linux, linux-ext4, linux-kernel,
	natechancellor, ndesaulniers, syzkaller-bugs, tytso

Hello,

syzbot tried to test the proposed patch but the build/boot failed:

failed to checkout kernel repo git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git on commit e360ba58d067a30a4e3e7d55ebdd919885a058d6: failed to run ["git" "fetch" "--tags" "d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8"]: exit status 1
From git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4
 * [new branch]                bisect-test-ext4-035     -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/bisect-test-ext4-035
 * [new branch]                bisect-test-generic-307  -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/bisect-test-generic-307
 * [new branch]                dev                      -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/dev
 * [new branch]                ext4-3.18                -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/ext4-3.18
 * [new branch]                ext4-4.1                 -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/ext4-4.1
 * [new branch]                ext4-4.4                 -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/ext4-4.4
 * [new branch]                ext4-4.9                 -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/ext4-4.9
 * [new branch]                ext4-dax                 -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/ext4-dax
 * [new branch]                ext4-tools               -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/ext4-tools
 * [new branch]                fix-bz-206443            -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/fix-bz-206443
 * [new branch]                for-stable               -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/for-stable
 * [new branch]                fsverity                 -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/fsverity
 * [new branch]                lazy_journal             -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/lazy_journal
 * [new branch]                master                   -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/master
 * [new branch]                origin                   -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/origin
 * [new branch]                pu                       -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/pu
 * [new branch]                test                     -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/test
 * [new tag]                   ext4-for-linus-5.8-rc1-2 -> ext4-for-linus-5.8-rc1-2
 ! [rejected]                  ext4_for_linus           -> ext4_for_linus  (would clobber existing tag)
 * [new tag]                   ext4_for_linus_bugfixes  -> ext4_for_linus_bugfixes
 * [new tag]                   ext4_for_linus_cleanups  -> ext4_for_linus_cleanups
 * [new tag]                   ext4_for_linus_fixes     -> ext4_for_linus_fixes
 * [new tag]                   ext4_for_linus_fixes2    -> ext4_for_linus_fixes2



Tested on:

commit:         [unknown 
git tree:       git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git e360ba58d067a30a4e3e7d55ebdd919885a058d6
dashboard link: https://syzkaller.appspot.com/bug?extid=345b75652b1d24227443
compiler:       gcc (GCC) 10.1.0-syz 20200507
patch:          https://syzkaller.appspot.com/x/patch.diff?x=1499c287500000


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: UBSAN: shift-out-of-bounds in ext4_fill_super
  2020-12-10  3:50   ` syzbot
@ 2020-12-10  8:09     ` Dmitry Vyukov
  2020-12-10 13:56       ` Dmitry Vyukov
  2020-12-10 18:28       ` Theodore Y. Ts'o
  0 siblings, 2 replies; 10+ messages in thread
From: Dmitry Vyukov @ 2020-12-10  8:09 UTC (permalink / raw)
  To: syzbot
  Cc: Andreas Dilger, clang-built-linux, linux-ext4, LKML,
	Nathan Chancellor, Nick Desaulniers, syzkaller-bugs,
	Theodore Ts'o

On Thu, Dec 10, 2020 at 4:50 AM syzbot
<syzbot+345b75652b1d24227443@syzkaller.appspotmail.com> wrote:
>
> Hello,
>
> syzbot tried to test the proposed patch but the build/boot failed:
>
> failed to checkout kernel repo git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git on commit e360ba58d067a30a4e3e7d55ebdd919885a058d6: failed to run ["git" "fetch" "--tags" "d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8"]: exit status 1
> From git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4
>  * [new branch]                bisect-test-ext4-035     -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/bisect-test-ext4-035
>  * [new branch]                bisect-test-generic-307  -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/bisect-test-generic-307
>  * [new branch]                dev                      -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/dev
>  * [new branch]                ext4-3.18                -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/ext4-3.18
>  * [new branch]                ext4-4.1                 -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/ext4-4.1
>  * [new branch]                ext4-4.4                 -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/ext4-4.4
>  * [new branch]                ext4-4.9                 -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/ext4-4.9
>  * [new branch]                ext4-dax                 -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/ext4-dax
>  * [new branch]                ext4-tools               -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/ext4-tools
>  * [new branch]                fix-bz-206443            -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/fix-bz-206443
>  * [new branch]                for-stable               -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/for-stable
>  * [new branch]                fsverity                 -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/fsverity
>  * [new branch]                lazy_journal             -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/lazy_journal
>  * [new branch]                master                   -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/master
>  * [new branch]                origin                   -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/origin
>  * [new branch]                pu                       -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/pu
>  * [new branch]                test                     -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/test
>  * [new tag]                   ext4-for-linus-5.8-rc1-2 -> ext4-for-linus-5.8-rc1-2
>  ! [rejected]                  ext4_for_linus           -> ext4_for_linus  (would clobber existing tag)

Interesting. First time I see this. Should syzkaller use 'git fetch
--tags --force"?...
StackOverflow suggests it should help:
https://stackoverflow.com/questions/58031165/how-to-get-rid-of-would-clobber-existing-tag


>  * [new tag]                   ext4_for_linus_bugfixes  -> ext4_for_linus_bugfixes
>  * [new tag]                   ext4_for_linus_cleanups  -> ext4_for_linus_cleanups
>  * [new tag]                   ext4_for_linus_fixes     -> ext4_for_linus_fixes
>  * [new tag]                   ext4_for_linus_fixes2    -> ext4_for_linus_fixes2
>
>
>
> Tested on:
>
> commit:         [unknown
> git tree:       git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git e360ba58d067a30a4e3e7d55ebdd919885a058d6
> dashboard link: https://syzkaller.appspot.com/bug?extid=345b75652b1d24227443
> compiler:       gcc (GCC) 10.1.0-syz 20200507
> patch:          https://syzkaller.appspot.com/x/patch.diff?x=1499c287500000

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: UBSAN: shift-out-of-bounds in ext4_fill_super
  2020-12-10  8:09     ` Dmitry Vyukov
@ 2020-12-10 13:56       ` Dmitry Vyukov
  2020-12-10 19:24         ` syzbot
  2020-12-10 18:28       ` Theodore Y. Ts'o
  1 sibling, 1 reply; 10+ messages in thread
From: Dmitry Vyukov @ 2020-12-10 13:56 UTC (permalink / raw)
  To: syzbot
  Cc: Andreas Dilger, clang-built-linux, linux-ext4, LKML,
	Nathan Chancellor, Nick Desaulniers, syzkaller-bugs,
	Theodore Ts'o

[-- Attachment #1: Type: text/plain, Size: 3235 bytes --]

On Thu, Dec 10, 2020 at 9:09 AM Dmitry Vyukov <dvyukov@google.com> wrote:
>
> On Thu, Dec 10, 2020 at 4:50 AM syzbot
> <syzbot+345b75652b1d24227443@syzkaller.appspotmail.com> wrote:
> >
> > Hello,
> >
> > syzbot tried to test the proposed patch but the build/boot failed:
> >
> > failed to checkout kernel repo git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git on commit e360ba58d067a30a4e3e7d55ebdd919885a058d6: failed to run ["git" "fetch" "--tags" "d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8"]: exit status 1
> > From git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4
> >  * [new branch]                bisect-test-ext4-035     -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/bisect-test-ext4-035
> >  * [new branch]                bisect-test-generic-307  -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/bisect-test-generic-307
> >  * [new branch]                dev                      -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/dev
> >  * [new branch]                ext4-3.18                -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/ext4-3.18
> >  * [new branch]                ext4-4.1                 -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/ext4-4.1
> >  * [new branch]                ext4-4.4                 -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/ext4-4.4
> >  * [new branch]                ext4-4.9                 -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/ext4-4.9
> >  * [new branch]                ext4-dax                 -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/ext4-dax
> >  * [new branch]                ext4-tools               -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/ext4-tools
> >  * [new branch]                fix-bz-206443            -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/fix-bz-206443
> >  * [new branch]                for-stable               -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/for-stable
> >  * [new branch]                fsverity                 -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/fsverity
> >  * [new branch]                lazy_journal             -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/lazy_journal
> >  * [new branch]                master                   -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/master
> >  * [new branch]                origin                   -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/origin
> >  * [new branch]                pu                       -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/pu
> >  * [new branch]                test                     -> d06f7b29746c7f0a52f349ff7fbf2a3f22d27cf8/test
> >  * [new tag]                   ext4-for-linus-5.8-rc1-2 -> ext4-for-linus-5.8-rc1-2
> >  ! [rejected]                  ext4_for_linus           -> ext4_for_linus  (would clobber existing tag)
>
> Interesting. First time I see this. Should syzkaller use 'git fetch
> --tags --force"?...
> StackOverflow suggests it should help:
> https://stackoverflow.com/questions/58031165/how-to-get-rid-of-would-clobber-existing-tag


I've added --force to fetches:
https://github.com/google/syzkaller/commit/9a72bc3440b65a01187ba4277b49d6bd821079cd
 and it should be deployed by now. Let's try again:

#syz test git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git
e360ba58d067a30a4e3e7d55ebdd919885a058d6

[-- Attachment #2: patch --]
[-- Type: application/octet-stream, Size: 3292 bytes --]

From 3d3bc303a8a8f7123cf486f49fa9060116fa1465 Mon Sep 17 00:00:00 2001
From: Theodore Ts'o <tytso@mit.edu>
Date: Wed, 9 Dec 2020 15:59:11 -0500
Subject: [PATCH] ext4: check for invalid block size early when mounting a file
 system

Check for valid block size directly by validating s_log_block_size; we
were doing this in two places.  First, by calculating blocksize via
BLOCK_SIZE << s_log_block_size, and then checking that the blocksize
was valid.  And then secondly, by checking s_log_block_size directly.

The first check is not reliable, and can trigger an UBSAN warning if
s_log_block_size on a maliciously corrupted superblock is greater than
22.  This is harmless, since the second test will correctly reject the
maliciously fuzzed file system, but to make syzbot shut up, and
because the two checks are duplicative in any case, delete the
blocksize check, and move the s_log_block_size earlier in
ext4_fill_super().

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Reported-by: syzbot+345b75652b1d24227443@syzkaller.appspotmail.com
---
 fs/ext4/super.c | 40 ++++++++++++++++------------------------
 1 file changed, 16 insertions(+), 24 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index f86220a8df50..4a16bbf0432c 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4202,18 +4202,25 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	 */
 	sbi->s_li_wait_mult = EXT4_DEF_LI_WAIT_MULT;
 
-	blocksize = BLOCK_SIZE << le32_to_cpu(es->s_log_block_size);
-
-	if (blocksize == PAGE_SIZE)
-		set_opt(sb, DIOREAD_NOLOCK);
-
-	if (blocksize < EXT4_MIN_BLOCK_SIZE ||
-	    blocksize > EXT4_MAX_BLOCK_SIZE) {
+	if (le32_to_cpu(es->s_log_block_size) >
+	    (EXT4_MAX_BLOCK_LOG_SIZE - EXT4_MIN_BLOCK_LOG_SIZE)) {
 		ext4_msg(sb, KERN_ERR,
-		       "Unsupported filesystem blocksize %d (%d log_block_size)",
-			 blocksize, le32_to_cpu(es->s_log_block_size));
+			 "Invalid log block size: %u",
+			 le32_to_cpu(es->s_log_block_size));
 		goto failed_mount;
 	}
+	if (le32_to_cpu(es->s_log_cluster_size) >
+	    (EXT4_MAX_CLUSTER_LOG_SIZE - EXT4_MIN_BLOCK_LOG_SIZE)) {
+		ext4_msg(sb, KERN_ERR,
+			 "Invalid log cluster size: %u",
+			 le32_to_cpu(es->s_log_cluster_size));
+		goto failed_mount;
+	}
+
+	blocksize = EXT4_MIN_BLOCK_SIZE << le32_to_cpu(es->s_log_block_size);
+
+	if (blocksize == PAGE_SIZE)
+		set_opt(sb, DIOREAD_NOLOCK);
 
 	if (le32_to_cpu(es->s_rev_level) == EXT4_GOOD_OLD_REV) {
 		sbi->s_inode_size = EXT4_GOOD_OLD_INODE_SIZE;
@@ -4432,21 +4439,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	if (!ext4_feature_set_ok(sb, (sb_rdonly(sb))))
 		goto failed_mount;
 
-	if (le32_to_cpu(es->s_log_block_size) >
-	    (EXT4_MAX_BLOCK_LOG_SIZE - EXT4_MIN_BLOCK_LOG_SIZE)) {
-		ext4_msg(sb, KERN_ERR,
-			 "Invalid log block size: %u",
-			 le32_to_cpu(es->s_log_block_size));
-		goto failed_mount;
-	}
-	if (le32_to_cpu(es->s_log_cluster_size) >
-	    (EXT4_MAX_CLUSTER_LOG_SIZE - EXT4_MIN_BLOCK_LOG_SIZE)) {
-		ext4_msg(sb, KERN_ERR,
-			 "Invalid log cluster size: %u",
-			 le32_to_cpu(es->s_log_cluster_size));
-		goto failed_mount;
-	}
-
 	if (le16_to_cpu(sbi->s_es->s_reserved_gdt_blocks) > (blocksize / 4)) {
 		ext4_msg(sb, KERN_ERR,
 			 "Number of reserved GDT blocks insanely large: %d",

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: UBSAN: shift-out-of-bounds in ext4_fill_super
  2020-12-10  8:09     ` Dmitry Vyukov
  2020-12-10 13:56       ` Dmitry Vyukov
@ 2020-12-10 18:28       ` Theodore Y. Ts'o
  2020-12-14 14:37         ` Dmitry Vyukov
  1 sibling, 1 reply; 10+ messages in thread
From: Theodore Y. Ts'o @ 2020-12-10 18:28 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: syzbot, Andreas Dilger, clang-built-linux, linux-ext4, LKML,
	Nathan Chancellor, Nick Desaulniers, syzkaller-bugs

On Thu, Dec 10, 2020 at 09:09:51AM +0100, Dmitry Vyukov wrote:
> >  * [new tag]                   ext4-for-linus-5.8-rc1-2 -> ext4-for-linus-5.8-rc1-2
> >  ! [rejected]                  ext4_for_linus           -> ext4_for_linus  (would clobber existing tag)
> 
> Interesting. First time I see this. Should syzkaller use 'git fetch
> --tags --force"?...
> StackOverflow suggests it should help:
> https://stackoverflow.com/questions/58031165/how-to-get-rid-of-would-clobber-existing-tag

Yeah, sorry, ext4_for_linus is a signed tag which is only used to
authenticate my pull request to Linus.  After Linus accepts the pull,
the digital signature is going to be upstream, and so I end up
deleting and the reusing that tag for the next merge window.

I guess I could just start always using ext4_for_linus-<VERSION> and
just delete the tags once they have been accepted, to keep my list of
tags clean. 

It's going to make everyone else's tags who pull from ext4.git messy,
though, with gobs of tags that probably won't be of use to them.  It
does avoid the need to use git fetch --tags --force, and I guess
people are used to the need to GC tags with the linux-repo.  So maybe
that's the right thing to do going forward.


	     	 	       	       	   	- Ted

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: UBSAN: shift-out-of-bounds in ext4_fill_super
  2020-12-10 13:56       ` Dmitry Vyukov
@ 2020-12-10 19:24         ` syzbot
  0 siblings, 0 replies; 10+ messages in thread
From: syzbot @ 2020-12-10 19:24 UTC (permalink / raw)
  To: adilger.kernel, clang-built-linux, dvyukov, linux-ext4,
	linux-kernel, natechancellor, ndesaulniers, syzkaller-bugs,
	tytso

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+345b75652b1d24227443@syzkaller.appspotmail.com

Tested on:

commit:         e360ba58 ext4: fix a memory leak of ext4_free_data
git tree:       git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git
kernel config:  https://syzkaller.appspot.com/x/.config?x=fe9725f8845d9fe6
dashboard link: https://syzkaller.appspot.com/bug?extid=345b75652b1d24227443
compiler:       gcc (GCC) 10.1.0-syz 20200507
patch:          https://syzkaller.appspot.com/x/patch.diff?x=1166cf17500000

Note: testing is done by a robot and is best-effort only.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: UBSAN: shift-out-of-bounds in ext4_fill_super
  2020-12-10 18:28       ` Theodore Y. Ts'o
@ 2020-12-14 14:37         ` Dmitry Vyukov
  2020-12-14 20:36           ` Theodore Y. Ts'o
  0 siblings, 1 reply; 10+ messages in thread
From: Dmitry Vyukov @ 2020-12-14 14:37 UTC (permalink / raw)
  To: Theodore Y. Ts'o
  Cc: syzbot, Andreas Dilger, clang-built-linux, linux-ext4, LKML,
	Nathan Chancellor, Nick Desaulniers, syzkaller-bugs

On Thu, Dec 10, 2020 at 7:28 PM Theodore Y. Ts'o <tytso@mit.edu> wrote:
>
> On Thu, Dec 10, 2020 at 09:09:51AM +0100, Dmitry Vyukov wrote:
> > >  * [new tag]                   ext4-for-linus-5.8-rc1-2 -> ext4-for-linus-5.8-rc1-2
> > >  ! [rejected]                  ext4_for_linus           -> ext4_for_linus  (would clobber existing tag)
> >
> > Interesting. First time I see this. Should syzkaller use 'git fetch
> > --tags --force"?...
> > StackOverflow suggests it should help:
> > https://stackoverflow.com/questions/58031165/how-to-get-rid-of-would-clobber-existing-tag
>
> Yeah, sorry, ext4_for_linus is a signed tag which is only used to
> authenticate my pull request to Linus.  After Linus accepts the pull,
> the digital signature is going to be upstream, and so I end up
> deleting and the reusing that tag for the next merge window.
>
> I guess I could just start always using ext4_for_linus-<VERSION> and
> just delete the tags once they have been accepted, to keep my list of
> tags clean.
>
> It's going to make everyone else's tags who pull from ext4.git messy,
> though, with gobs of tags that probably won't be of use to them.  It
> does avoid the need to use git fetch --tags --force, and I guess
> people are used to the need to GC tags with the linux-repo.  So maybe
> that's the right thing to do going forward.

Hi Ted,

syzbot is now prepared and won't fail next time, nor on other similar
trees. Which is good.
So it's really up to you.

Thanks

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: UBSAN: shift-out-of-bounds in ext4_fill_super
  2020-12-14 14:37         ` Dmitry Vyukov
@ 2020-12-14 20:36           ` Theodore Y. Ts'o
  2020-12-15 10:09             ` Dmitry Vyukov
  0 siblings, 1 reply; 10+ messages in thread
From: Theodore Y. Ts'o @ 2020-12-14 20:36 UTC (permalink / raw)
  To: Dmitry Vyukov; +Cc: Andreas Dilger, LKML, Nathan Chancellor, Nick Desaulniers

(Dropping off-topic lists)

On Mon, Dec 14, 2020 at 03:37:37PM +0100, Dmitry Vyukov wrote:
> > It's going to make everyone else's tags who pull from ext4.git messy,
> > though, with gobs of tags that probably won't be of use to them.  It
> > does avoid the need to use git fetch --tags --force, and I guess
> > people are used to the need to GC tags with the linux-repo.

(I had meant to say linux-next repo above.)

> syzbot is now prepared and won't fail next time, nor on other similar
> trees. Which is good.
> So it's really up to you.

I'm curious --- are you having to do anything special in terms of
deleting old tags to keep the size of the repo under control?  Git
will keep a tag around indefinitely, so if you have huge numbers of
next-YYYYMMDD tags in your repo, the size will grow without bound.
Are you doing anything to automatically garbage collect tags to preven
this from being a problem?

(I am not pulling linux-next every day; only when I need to debug a
bug reported against the -next tree, so I just manually delete the
tags as necessary.  So I'm curious what folks who are following
linux-next are doing, and whether they have something specific for
linux-next tags, or whether they have a more general solution.)

Cheers,

					- Ted

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: UBSAN: shift-out-of-bounds in ext4_fill_super
  2020-12-14 20:36           ` Theodore Y. Ts'o
@ 2020-12-15 10:09             ` Dmitry Vyukov
  0 siblings, 0 replies; 10+ messages in thread
From: Dmitry Vyukov @ 2020-12-15 10:09 UTC (permalink / raw)
  To: Theodore Y. Ts'o
  Cc: Andreas Dilger, LKML, Nathan Chancellor, Nick Desaulniers

On Mon, Dec 14, 2020 at 9:36 PM Theodore Y. Ts'o <tytso@mit.edu> wrote:
>
> (Dropping off-topic lists)
>
> On Mon, Dec 14, 2020 at 03:37:37PM +0100, Dmitry Vyukov wrote:
> > > It's going to make everyone else's tags who pull from ext4.git messy,
> > > though, with gobs of tags that probably won't be of use to them.  It
> > > does avoid the need to use git fetch --tags --force, and I guess
> > > people are used to the need to GC tags with the linux-repo.
>
> (I had meant to say linux-next repo above.)
>
> > syzbot is now prepared and won't fail next time, nor on other similar
> > trees. Which is good.
> > So it's really up to you.
>
> I'm curious --- are you having to do anything special in terms of
> deleting old tags to keep the size of the repo under control?  Git
> will keep a tag around indefinitely, so if you have huge numbers of
> next-YYYYMMDD tags in your repo, the size will grow without bound.
> Are you doing anything to automatically garbage collect tags to preven
> this from being a problem?
>
> (I am not pulling linux-next every day; only when I need to debug a
> bug reported against the -next tree, so I just manually delete the
> tags as necessary.  So I'm curious what folks who are following
> linux-next are doing, and whether they have something specific for
> linux-next tags, or whether they have a more general solution.)
>
> Cheers,
>
>                                         - Ted


syzbot does not do anything special here, it just polls/fetches always.

Here are sizes of checkouts that it has now, these also include
in-tree builds, that may explain larger differences. They don't look
too bad.

2.8G upstream-bpf-kasan-gce/kernel
3.1G upstream-bpf-next-kasan-gce/kernel
5.1G upstream-gce-leak/kernel
6.3G upstream-kasan-gce/kernel
6.3G upstream-kasan-gce-386/kernel
6.3G upstream-kasan-gce-root/kernel
6.3G upstream-kasan-gce-selinux-root/kernel
9.3G upstream-kasan-gce-smack-root/kernel
2.8G upstream-kmsan-gce/kernel
2.8G upstream-kmsan-gce-386/kernel
2.9G upstream-linux-next-kasan-gce-root/kernel
6.3G upstream-net-kasan-gce/kernel
2.7G upstream-net-this-kasan-gce/kernel
5.5G android-414-kasan-gce-root/kernel
6.5G android-44-kasan-gce/kernel
6.5G android-44-kasan-gce-386/kernel
6.0G android-49-kasan-gce/kernel
6.0G android-49-kasan-gce-386/kernel
6.1G android-49-kasan-gce-root/kernel

And the one is used for all patch testing on random trees is 5.2G, it has:

kernel$ git remote -v | wc -l
60
kernel$ git tag -l  | wc -l
7544

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-12-15 10:10 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-09  7:33 UBSAN: shift-out-of-bounds in ext4_fill_super syzbot
2020-12-10  2:36 ` Theodore Y. Ts'o
2020-12-10  3:50   ` syzbot
2020-12-10  8:09     ` Dmitry Vyukov
2020-12-10 13:56       ` Dmitry Vyukov
2020-12-10 19:24         ` syzbot
2020-12-10 18:28       ` Theodore Y. Ts'o
2020-12-14 14:37         ` Dmitry Vyukov
2020-12-14 20:36           ` Theodore Y. Ts'o
2020-12-15 10:09             ` Dmitry Vyukov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.