On Sun 30-04-23 11:43:10, Theodore Ts'o wrote: > Previously, ext4_get_group_info() would treat an invalid group number > as BUG(), since in theory it should never happen. However, if a > malicious attaker (or fuzzer) modifies the superblock via the block > device while it is the file system is mounted, it is possible for > s_first_data_block to get set to a very large number. In that case, > when calculating the block group of some block number (such as the > starting block of a preallocation region), could result in an > underflow and very large block group number. Then the BUG_ON check in > ext4_get_group_info() would fire, resutling in a denial of service > attack that can be triggered by root or someone with write access to > the block device. > > For a quality of implementation perspective, it's best that even if > the system administrator does something that they shouldn't, that it > will not trigger a BUG. So instead of BUG'ing, ext4_get_group_info() > will call ext4_error and return NULL. We also add fallback code in > all of the callers of ext4_get_group_info() that it might NULL. > > Also, since ext4_get_group_info() was already borderline to be an > inline function, un-inline it. The results in a next reduction of the > compiled text size of ext4 by roughly 2k. > > Reported-by: syzbot+e2efa3efc15a1c9e95c3@xxxxxxxxxxxxxxxxxxxxxxxxx > Link: https://syzkaller.appspot.com/bug?id=69b28112e098b070f639efb356393af3ffec4220 > Signed-off-by: Theodore Ts'o <tytso@xxxxxxx> The patch looks good except for one small problem already found by Julia: > @@ -2578,7 +2595,7 @@ void ext4_mb_prefetch_fini(struct super_block *sb, ext4_group_t group, > gdp = ext4_get_group_desc(sb, group, NULL); > grp = ext4_get_group_info(sb, group); > > - if (EXT4_MB_GRP_NEED_INIT(grp) && > + if (grp && grp && EXT4_MB_GRP_NEED_INIT(grp) && ^^^ one of these should be gdp. With this fixed feel free to add: Reviewed-by: Jan Kara <jack@xxxxxxx> Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
