ext4: Fix possible NULL pointer dereference in ext4_group_desc_free()

This can happen if the kvmalloc_objs() fails and sbi->s_group_desc pointer is NULL in the ext4_group_desc_init(), and then the ext4_group_desc_free() is called, leading to a NULL group_desc pointer dereference.

This commit therefore adds a NULL check for sbi->s_group_desc before accessing its internal members.

Signed-off-by Zqiang <[email protected]>

---
 fs/ext4/super.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

fs/ext4/super.c +0 -0

index 43f680c750ae..c4307dc04687 100644

fs/ext4/super.c +5 -2

-		brelse(group_desc[i]);
-	kvfree(group_desc);
+	if (group_desc) {
+		for (i = 0; i < sbi->s_gdb_count; i++)
+			brelse(group_desc[i]);
+		kvfree(group_desc);
+	}
 	rcu_read_unlock();
 }
 

Quoted 20 lines

This can happen if the kvmalloc_objs() fails and sbi->s_group_desc pointer
is NULL in the ext4_group_desc_init(), and then the ext4_group_desc_free() is called, leading to a NULL group_desc pointer dereference.

This commit therefore adds a NULL check for sbi->s_group_desc before accessing its internal members.

Signed-off-by Zqiang <[email protected]>

---

 fs/ext4/super.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 43f680c750ae..c4307dc04687 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1256,9 +1256,11 @@ static void ext4_group_desc_free(struct ext4_sb_info *sbi)

Quoted 4 lines

rcu_read_lock();
group_desc = rcu_dereference(sbi->s_group_desc);
- for (i = 0; i < sbi->s_gdb_count; i++)

In ext4_group_desc_init(), s_gdb_count is only assigned after kvmalloc_array allocation succeeds. Therefore, when kvmalloc_array fails, the brelse(group_desc[i]) in ext4_group_desc_free() will not actually be executed,
and thus this NULL pointer dereference issue will not be triggered.

Cheers,
Baokun

Quoted 11 lines

- brelse(group_desc[i]);
- kvfree(group_desc);
+ if (group_desc) {
+ for (i = 0; i < sbi->s_gdb_count; i++)
+ brelse(group_desc[i]);
+ kvfree(group_desc);
+ }
rcu_read_unlock();
}

Quoted 24 lines · Zqiang

Quoted 19 lines

This can happen if the kvmalloc_objs() fails and sbi->s_group_desc pointer
is NULL in the ext4_group_desc_init(), and then the ext4_group_desc_free() is called, leading to a NULL group_desc pointer dereference.

This commit therefore adds a NULL check for sbi->s_group_desc before accessing its internal members.

Signed-off-by Zqiang <[email protected]>

---

 fs/ext4/super.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 43f680c750ae..c4307dc04687 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1256,9 +1256,11 @@ static void ext4_group_desc_free(struct ext4_sb_info *sbi)

Quoted 4 lines

rcu_read_lock();
group_desc = rcu_dereference(sbi->s_group_desc);
- for (i = 0; i < sbi->s_gdb_count; i++)

Quoted 5 lines

In ext4_group_desc_init(), s_gdb_count is only assigned after kvmalloc_array allocation succeeds. Therefore, when kvmalloc_array fails, the brelse(group_desc[i]) in ext4_group_desc_free() will not actually be executed,
and thus this NULL pointer dereference issue will not be triggered.

Thanks for replay, got it, sorry for make noise.

Just then, I find that warning may be trigger:

the kvfree() is called in RCU read critical section, if the sbi->s_group_desc pointer comes from vmalloc(), the vfree() is called to release it, but the might_sleep() is called in the vfree(), this may be trigger warnings in rcu_sleep_check() when the enable CONFIG_DEBUG_ATOMIC_SLEEP.

May be use rcu_access_pointer() to access sbi->s_group_desc is enough.

Quoted 36 lines

Quoted 5 lines · Zqiang

This can happen if the kvmalloc_objs() fails and sbi->s_group_desc pointer is NULL in the ext4_group_desc_init(), and then the ext4_group_desc_free() is called, leading to a NULL group_desc pointer dereference.

This commit therefore adds a NULL check for sbi->s_group_desc before accessing its internal members.

Signed-off-by Zqiang <[email protected]>

---

 fs/ext4/super.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 43f680c750ae..c4307dc04687 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1256,9 +1256,11 @@ static void ext4_group_desc_free(struct ext4_sb_info *sbi)

rcu_read_lock();
group_desc = rcu_dereference(sbi->s_group_desc);
- for (i = 0; i < sbi->s_gdb_count; i++)

Quoted 7 lines

In ext4_group_desc_init(), s_gdb_count is only assigned after kvmalloc_array
allocation succeeds. Therefore, when kvmalloc_array fails, the brelse(group_desc[i]) in ext4_group_desc_free() will not actually be executed,
and thus this NULL pointer dereference issue will not be triggered.

Thanks for replay, got it, sorry for make noise.

Quoted 8 lines

Just then, I find that warning may be trigger:

the kvfree() is called in RCU read critical section, if the sbi->s_group_desc pointer comes from vmalloc(), the vfree() is called to release it, but the might_sleep() is called in the vfree(), this may be trigger warnings in rcu_sleep_check() when the enable CONFIG_DEBUG_ATOMIC_SLEEP.

Indeed, vfree triggers the following warning: ```
===========================================================================
EXT4-fs (vdc): unmounting filesystem c478da00-c52c-4dd4-81c1-d4f93e12ab50. BUG: sleeping function called from invalid context at mm/vmalloc.c:3441 in_atomic(): 0, irqs_disabled(): 0,
non_block: 0, pid: 457, name: umount preempt_count: 0, expected: 0 RCU nest depth: 1, expected: 0 CPU: 0 UID: 0 PID: 457 Comm: umount Tainted:
G W 6.19.0-rc4-g4f5e8e6f0123-dirty #10 PREEMPT(none) Tainted: [W]=WARN

Call Trace: <TASK> dump_stack_lvl+0x55/0x70 __might_resched+0x116/0x160
vfree+0x38/0x60 ext4_put_super+0x1ac/0x490
generic_shutdown_super+0x81/0x180 kill_block_super+0x1a/0x40
ext4_kill_sb+0x22/0x40 deactivate_locked_super+0x35/0xb0
cleanup_mnt+0x101/0x170 task_work_run+0x5c/0xa0
exit_to_user_mode_loop+0xe2/0x460 do_syscall_64+0x1de/0x1f0
entry_SYSCALL_64_after_hwframe+0x76/0x7e

===========================================================================
``` And ext4_mb_init_backend, ext4_mb_release, ext4_flex_groups_free have similar issues.Indeed, vfree triggers the following warning:

===========================================================================
EXT4-fs (vdc): unmounting filesystem c478da00-c52c-4dd4-81c1-d4f93e12ab50.

BUG: sleeping function called from invalid context at mm/vmalloc.c:3441
in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid: 457, name: umount
preempt_count: 0, expected: 0
RCU nest depth: 1, expected: 0
CPU: 0 UID: 0 PID: 457 Comm: umount Tainted: G W
               6.19.0-rc4-g4f5e8e6f0123-dirty #10 PREEMPT(none)
Tainted: [W]=WARN
Call Trace:
 <TASK>
 dump_stack_lvl+0x55/0x70
 __might_resched+0x116/0x160
 vfree+0x38/0x60
 ext4_put_super+0x1ac/0x490
 generic_shutdown_super+0x81/0x180
 kill_block_super+0x1a/0x40
 ext4_kill_sb+0x22/0x40
 deactivate_locked_super+0x35/0xb0
 cleanup_mnt+0x101/0x170
 task_work_run+0x5c/0xa0
 exit_to_user_mode_loop+0xe2/0x460
 do_syscall_64+0x1de/0x1f0
 entry_SYSCALL_64_after_hwframe+0x76/0x7e
===========================================================================

And ext4_mb_init_backend, ext4_mb_release, ext4_flex_groups_free have similar issues.

Quoted 1 line

Quoted 3 lines

May be use rcu_access_pointer() to access sbi->s_group_desc
is enough.

Yes, these are all initialization or teardown paths and cannot run concurrently with
online resize, so using rcu_access_pointer() seems sufficient.

Also, should we add might_sleep() to kvfree() to prevent similar issues?

Cheers,
Baokun