[PATCH v2] ext4: fix a NULL pointer when validating an inode bitmap
From: Luís Henriques
Date: Tue Oct 11 2022 - 11:56:04 EST
It's possible to hit a NULL pointer exception while accessing the
sb->s_group_info in ext4_validate_inode_bitmap(), when calling
ext4_get_group_info().
EXT4-fs (loop0): warning: mounting unchecked fs, running e2fsck is recommended
EXT4-fs error (device loop0): ext4_clear_blocks:866: inode #32: comm mount: attempt to clear invalid blocks 16777450 len 1
EXT4-fs error (device loop0): ext4_free_branches:1012: inode #32: comm mount: invalid indirect mapped block 1258291200 (level 1)
EXT4-fs error (device loop0): ext4_free_branches:1012: inode #32: comm mount: invalid indirect mapped block 7379847 (level 2)
BUG: kernel NULL pointer dereference, address: 0000000000000000
...
RIP: 0010:ext4_read_inode_bitmap+0x21b/0x5a0
...
Call Trace:
<TASK>
ext4_free_inode+0x172/0x5c0
ext4_evict_inode+0x4a5/0x730
evict+0xc1/0x1c0
ext4_setup_system_zone+0x2ea/0x380
ext4_fill_super+0x249f/0x3910
? ext4_reconfigure+0x880/0x880
? snprintf+0x49/0x60
? ext4_reconfigure+0x880/0x880
get_tree_bdev+0x169/0x260
vfs_get_tree+0x16/0x70
path_mount+0x77d/0xa30
__x64_sys_mount+0x101/0x140
do_syscall_64+0x3c/0x80
entry_SYSCALL_64_after_hwframe+0x46/0xb0
This issue can be fixed by returning NULL in ext4_get_group_info() when
->s_group_info is NULL. This also requires checking the return code from
ext4_get_group_info() when appropriate.
While there, also ensure the right error code (-EFSCORRUPTED) is propagated
to user-space. EUCLEAN is more informative than ENOMEM.
CC: stable@xxxxxxxxxxxxxxx
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216541
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216539
Signed-off-by: Luís Henriques <lhenriques@xxxxxxx>
---
* Changes since v1:
I found out another bugzilla with the same issue (#216539), but on a
different place. The pattern was the same: a call to
ext4_get_group_info() followed by a EXT4_MB_GRP_*_CORRUPT check. I've
grep'ed the code and added the same check in (hopefully) all of them.
fs/ext4/balloc.c | 2 +-
fs/ext4/ext4.h | 17 ++++++++++-------
fs/ext4/ialloc.c | 6 +++---
fs/ext4/indirect.c | 14 ++++++++++++--
fs/ext4/mballoc.c | 9 +++++----
5 files changed, 31 insertions(+), 17 deletions(-)
diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
index 8ff4b9192a9f..1af1fc8b1891 100644
--- a/fs/ext4/balloc.c
+++ b/fs/ext4/balloc.c
@@ -377,7 +377,7 @@ static int ext4_validate_block_bitmap(struct super_block *sb,
if (buffer_verified(bh))
return 0;
- if (EXT4_MB_GRP_BBITMAP_CORRUPT(grp))
+ if (!grp || EXT4_MB_GRP_BBITMAP_CORRUPT(grp))
return -EFSCORRUPTED;
ext4_lock_group(sb, block_group);
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index e5f2f5ca5120..1c8b5876a28a 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -3324,13 +3324,16 @@ static inline
struct ext4_group_info *ext4_get_group_info(struct super_block *sb,
ext4_group_t group)
{
- struct ext4_group_info **grp_info;
- long indexv, indexh;
- BUG_ON(group >= EXT4_SB(sb)->s_groups_count);
- indexv = group >> (EXT4_DESC_PER_BLOCK_BITS(sb));
- indexh = group & ((EXT4_DESC_PER_BLOCK(sb)) - 1);
- grp_info = sbi_array_rcu_deref(EXT4_SB(sb), s_group_info, indexv);
- return grp_info[indexh];
+ struct ext4_group_info **grp_info;
+ long indexv, indexh;
+
+ BUG_ON(group >= EXT4_SB(sb)->s_groups_count);
+ if (!EXT4_SB(sb)->s_group_info)
+ return NULL;
+ indexv = group >> (EXT4_DESC_PER_BLOCK_BITS(sb));
+ indexh = group & ((EXT4_DESC_PER_BLOCK(sb)) - 1);
+ grp_info = sbi_array_rcu_deref(EXT4_SB(sb), s_group_info, indexv);
+ return grp_info[indexh];
}
/*
diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index 208b87ce8858..079b9c3af327 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -91,7 +91,7 @@ static int ext4_validate_inode_bitmap(struct super_block *sb,
if (buffer_verified(bh))
return 0;
- if (EXT4_MB_GRP_IBITMAP_CORRUPT(grp))
+ if (!grp || EXT4_MB_GRP_IBITMAP_CORRUPT(grp))
return -EFSCORRUPTED;
ext4_lock_group(sb, block_group);
@@ -293,7 +293,7 @@ void ext4_free_inode(handle_t *handle, struct inode *inode)
}
if (!(sbi->s_mount_state & EXT4_FC_REPLAY)) {
grp = ext4_get_group_info(sb, block_group);
- if (unlikely(EXT4_MB_GRP_IBITMAP_CORRUPT(grp))) {
+ if (unlikely(!grp || EXT4_MB_GRP_IBITMAP_CORRUPT(grp))) {
fatal = -EFSCORRUPTED;
goto error_return;
}
@@ -1048,7 +1048,7 @@ struct inode *__ext4_new_inode(struct user_namespace *mnt_userns,
* Skip groups with already-known suspicious inode
* tables
*/
- if (EXT4_MB_GRP_IBITMAP_CORRUPT(grp))
+ if (!grp || EXT4_MB_GRP_IBITMAP_CORRUPT(grp))
goto next_group;
}
diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c
index 860fc5119009..e5ac5c2363df 100644
--- a/fs/ext4/indirect.c
+++ b/fs/ext4/indirect.c
@@ -148,6 +148,7 @@ static Indirect *ext4_get_branch(struct inode *inode, int depth,
struct super_block *sb = inode->i_sb;
Indirect *p = chain;
struct buffer_head *bh;
+ unsigned int key;
int ret = -EIO;
*err = 0;
@@ -156,9 +157,18 @@ static Indirect *ext4_get_branch(struct inode *inode, int depth,
if (!p->key)
goto no_block;
while (--depth) {
- bh = sb_getblk(sb, le32_to_cpu(p->key));
+ key = le32_to_cpu(p->key);
+ bh = sb_getblk(sb, key);
if (unlikely(!bh)) {
- ret = -ENOMEM;
+ /*
+ * sb_getblk() masks different errors by always
+ * returning NULL. Let's distinguish at least the case
+ * where the block is out of range.
+ */
+ if (key > ext4_blocks_count(EXT4_SB(sb)->s_es))
+ ret = -EFSCORRUPTED;
+ else
+ ret = -ENOMEM;
goto failure;
}
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 9dad93059945..577e4d7415c3 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2386,7 +2386,7 @@ static bool ext4_mb_good_group(struct ext4_allocation_context *ac,
BUG_ON(cr < 0 || cr >= 4);
- if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(grp)))
+ if (unlikely(!grp || EXT4_MB_GRP_BBITMAP_CORRUPT(grp)))
return false;
free = grp->bb_free;
@@ -2466,7 +2466,7 @@ static int ext4_mb_good_group_nolock(struct ext4_allocation_context *ac,
goto out;
if (cr <= 2 && free < ac->ac_g_ex.fe_len)
goto out;
- if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(grp)))
+ if (unlikely(!grp || EXT4_MB_GRP_BBITMAP_CORRUPT(grp)))
goto out;
if (should_lock) {
__acquire(ext4_group_lock_ptr(sb, group));
@@ -5895,6 +5895,7 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
ext4_grpblk_t bit;
struct buffer_head *gd_bh;
ext4_group_t block_group;
+ struct ext4_group_info *grp_info;
struct ext4_sb_info *sbi;
struct ext4_buddy e4b;
unsigned int count_clusters;
@@ -5916,8 +5917,8 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
overflow = 0;
ext4_get_group_no_and_offset(sb, block, &block_group, &bit);
- if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(
- ext4_get_group_info(sb, block_group))))
+ grp_info = ext4_get_group_info(sb, block_group);
+ if (unlikely(!grp_info || EXT4_MB_GRP_BBITMAP_CORRUPT(grp_info)))
return;
/*