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

[Baokun Li]

On 2025-08-25 12:02, Ritesh Harjani (IBM) wrote:
> libaokun@xxxxxxxxxxxxxxx writes:
>
>> From: Baokun Li <libaokun1@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
>> Closes: https://syzkaller.appspot.com/bug?extid=1713b1aa266195b916c2
>> Fixes: f7eaacbb4e54 ("ext4: convert free groups order lists to xarrays")
>> Signed-off-by: Baokun Li <libaokun1@xxxxxxxxxx>
>> ---
>>  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 this code was introduced by me, when I saw the syzbot report,
I immediately realized that my previous changes had unintentionally
removed the null pointer check...
> Since these two functions can get called in error handling routine, we
> should check for null before iterating over the uninitalized array.
Yes, when making the changes, I missed the implicit null pointer check
within kfree. Sorry for this oversight.
>
> The fix looks good to me. Please feel free to add:
> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@xxxxxxxxx>
>
> -ritesh
>
Thanks for the review!


Cheers,
Baokun

>> 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