Re: [PATCH] ext4: fix potential null deref in ext4_mb_init()

[Ritesh Harjani (IBM) <ritesh.list@gmail.com>]

libaokun@huaweicloud.com writes:

> From: Baokun Li <libaokun1@huawei.com>
>
> In ext4_mb_init(), ext4_mb_avg_fragment_size_destroy() may be called
> when sbi->s_mb_avg_fragment_size remains uninitialized (e.g., if groupinfo
> slab cache allocation fails). Since ext4_mb_avg_fragment_size_destroy()
> lacks null pointer checking, this leads to a null pointer dereference.
>
> ==================================================================
> EXT4-fs: no memory for groupinfo slab cache
> BUG: kernel NULL pointer dereference, address: 0000000000000000
> PGD 0 P4D 0
> Oops: Oops: 0002 [#1] SMP PTI
> CPU:2 UID: 0 PID: 87 Comm:mount Not tainted 6.17.0-rc2 #1134 PREEMPT(none)
> RIP: 0010:_raw_spin_lock_irqsave+0x1b/0x40
> Call Trace:
>  <TASK>
>  xa_destroy+0x61/0x130
>  ext4_mb_init+0x483/0x540
>  __ext4_fill_super+0x116d/0x17b0
>  ext4_fill_super+0xd3/0x280
>  get_tree_bdev_flags+0x132/0x1d0
>  vfs_get_tree+0x29/0xd0
>  do_new_mount+0x197/0x300
>  __x64_sys_mount+0x116/0x150
>  do_syscall_64+0x50/0x1c0
>  entry_SYSCALL_64_after_hwframe+0x76/0x7e
> ==================================================================
>
> Therefore, add necessary null check to ext4_mb_avg_fragment_size_destroy()
> to prevent this issue. The same fix is also applied to
> ext4_mb_largest_free_orders_destroy().
>
> Reported-by: syzbot+1713b1aa266195b916c2@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=1713b1aa266195b916c2
> Fixes: f7eaacbb4e54 ("ext4: convert free groups order lists to xarrays")
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> ---
>  fs/ext4/mballoc.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)

I was just looking at the syzbot report. You submitted the fix pretty
fast ;)

Since these two functions can get called in error handling routine, we
should check for null before iterating over the uninitalized array.

The fix looks good to me. Please feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

-ritesh


>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 5898d92ba19f..6070d3c86678 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -3655,16 +3655,26 @@ static void ext4_discard_work(struct work_struct *work)
>  
>  static inline void ext4_mb_avg_fragment_size_destroy(struct ext4_sb_info *sbi)
>  {
> +	if (!sbi->s_mb_avg_fragment_size)
> +		return;
> +
>  	for (int i = 0; i < MB_NUM_ORDERS(sbi->s_sb); i++)
>  		xa_destroy(&sbi->s_mb_avg_fragment_size[i]);
> +
>  	kfree(sbi->s_mb_avg_fragment_size);
> +	sbi->s_mb_avg_fragment_size = NULL;
>  }
>  
>  static inline void ext4_mb_largest_free_orders_destroy(struct ext4_sb_info *sbi)
>  {
> +	if (!sbi->s_mb_largest_free_orders)
> +		return;
> +
>  	for (int i = 0; i < MB_NUM_ORDERS(sbi->s_sb); i++)
>  		xa_destroy(&sbi->s_mb_largest_free_orders[i]);
> +
>  	kfree(sbi->s_mb_largest_free_orders);
> +	sbi->s_mb_largest_free_orders = NULL;
>  }
>  
>  int ext4_mb_init(struct super_block *sb)
> -- 
> 2.46.1