| Message ID | 20230430154311.579720-3-tytso@mit.edu |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | ext4: fix syzbot report: kernel BUG in ext4_get_group_info | expand |
On Sun 30-04-23 11:43:11, Theodore Ts'o wrote: > If a malicious fuzzer overwrites the ext4 superblock while it is > mounted such that the s_first_data_block is set to a very large > number, the calculation of the block group can underflow, and trigger > a BUG_ON check. Change this to be an ext4_warning so that we don't > crash the kernel. > > Reported-by: syzbot+e2efa3efc15a1c9e95c3@syzkaller.appspotmail.com > Link: https://syzkaller.appspot.com/bug?id=69b28112e098b070f639efb356393af3ffec4220 > Signed-off-by: Theodore Ts'o <tytso@mit.edu> OK, looks good to me. But frankly there are many other interesting ways how bogus group numbers output when this happens can return is fun stuff - e.g. ext4_group_first_block_no() is going to return invalid blocks etc. So it feels a bit like endless whack-a-mole game. Anyway I agree the series seem to fix a big chunk of these sites so feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > fs/ext4/mballoc.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c > index dc13734f399d..9c7881a4ea75 100644 > --- a/fs/ext4/mballoc.c > +++ b/fs/ext4/mballoc.c > @@ -5047,7 +5047,11 @@ ext4_mb_release_group_pa(struct ext4_buddy *e4b, > trace_ext4_mb_release_group_pa(sb, pa); > BUG_ON(pa->pa_deleted == 0); > ext4_get_group_no_and_offset(sb, pa->pa_pstart, &group, &bit); > - BUG_ON(group != e4b->bd_group && pa->pa_len != 0); > + if (unlikely(group != e4b->bd_group && pa->pa_len != 0)) { > + ext4_warning(sb, "bad group: expected %u, group %u, pa_start %llu", > + e4b->bd_group, group, pa->pa_pstart); > + return 0; > + } > mb_free_blocks(pa->pa_inode, e4b, bit, pa->pa_len); > atomic_add(pa->pa_len, &EXT4_SB(sb)->s_mb_discarded); > trace_ext4_mballoc_discard(sb, NULL, group, bit, pa->pa_len); > -- > 2.31.0 >
On Sun, May 07, 2023 at 08:28:33PM +0200, Jan Kara wrote: > OK, looks good to me. But frankly there are many other interesting ways how > bogus group numbers output when this happens can return is fun stuff - e.g. > ext4_group_first_block_no() is going to return invalid blocks etc. So it > feels a bit like endless whack-a-mole game. Anyway I agree the series seem > to fix a big chunk of these sites so feel free to add: The main thing I'm trying to avoid is a kernel crash or possible out-of-bounds read or write, which could lead to a security vulnerability. If a a bad block number being returned by. say, ext4_group_first_block_no() "only" results in an I/O error when or (further) corruption of the block device, that's not a problem as far as I'm concerned. After all, if a malicious root access has read/write access to the block device, they can corrupt the file system *anyway*. I wasn't able to find cases where a crazy return value from ext4_group_first_block_no() which would cause a BUG or a buffer overrun. If we (or syzbot) finds a case where this could happen, we could copy s_es->s_first_data_block to sbi->s_first_data_block and then validate it during the file system mount. I also did a quick spot check what nastiness could be caused by real-time frobbing of s_blocks_count s_inodes_count and couldn't find anything there either. So it looks like s_first_data_block is the one which is the most problematic. Cheers, - Ted
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c index dc13734f399d..9c7881a4ea75 100644 --- a/fs/ext4/mballoc.c +++ b/fs/ext4/mballoc.c @@ -5047,7 +5047,11 @@ ext4_mb_release_group_pa(struct ext4_buddy *e4b, trace_ext4_mb_release_group_pa(sb, pa); BUG_ON(pa->pa_deleted == 0); ext4_get_group_no_and_offset(sb, pa->pa_pstart, &group, &bit); - BUG_ON(group != e4b->bd_group && pa->pa_len != 0); + if (unlikely(group != e4b->bd_group && pa->pa_len != 0)) { + ext4_warning(sb, "bad group: expected %u, group %u, pa_start %llu", + e4b->bd_group, group, pa->pa_pstart); + return 0; + } mb_free_blocks(pa->pa_inode, e4b, bit, pa->pa_len); atomic_add(pa->pa_len, &EXT4_SB(sb)->s_mb_discarded); trace_ext4_mballoc_discard(sb, NULL, group, bit, pa->pa_len);
If a malicious fuzzer overwrites the ext4 superblock while it is mounted such that the s_first_data_block is set to a very large number, the calculation of the block group can underflow, and trigger a BUG_ON check. Change this to be an ext4_warning so that we don't crash the kernel. Reported-by: syzbot+e2efa3efc15a1c9e95c3@syzkaller.appspotmail.com Link: https://syzkaller.appspot.com/bug?id=69b28112e098b070f639efb356393af3ffec4220 Signed-off-by: Theodore Ts'o <tytso@mit.edu> --- fs/ext4/mballoc.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)