Re: [PATCH] ext4: validate extent root during fast commit replay

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Critical] Incomplete fix allows type confusion and out-of-bounds access for indirect-mapped inodes during fast commit replay.

Pre-existing issues:
- [High] Pre-existing error pointer dereference leads to a guaranteed kernel panic, exposed by the new validation check.
--

--- Patch [1]: [PATCH] ext4: validate extent root during fast commit replay ---
commit 03dbc5f9111ce7346c9dc6815dd71904c9c82a55
Author: Jérémy Jean <[email protected]>

ext4: validate extent root during fast commit replay

This commit adds extent-tree validation for extent-formatted inode roots in
ext4_find_extent() during fast commit replay. It prevents out-of-bounds
memory writes caused by unvalidated extent roots with invalid entry counts.
This addresses an issue where ext4_iget() skips normal validation during
replay.

> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 559100f3e23cd..50484f7c3f25e 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -850,6 +850,13 @@ ext4_find_extent(struct inode *inode, ext4_lblk_t block,
>  		ret = -EFSCORRUPTED;
>  		goto err;
>  	}
> +	/* ext4_iget() skips extent validation during fast commit replay. */
> +	if (unlikely((EXT4_SB(inode->i_sb)->s_mount_state & EXT4_FC_REPLAY) &&
> +		     ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))) {

[Severity: Critical]
Could scoping the validation to extent-mapped inodes allow indirect-mapped
inodes to bypass the check?

During fast commit replay, ext4_fc_replay_add_range() unconditionally calls
ext4_find_extent() without ensuring the inode is actually extent-mapped:

fs/ext4/fast_commit.c:ext4_fc_replay_add_range() {
	...
		if (ret == 0) {
			/* Range is not mapped */
			path = ext4_find_extent(inode, cur, NULL, 0);
	...
}

If an attacker crafts an ext4 image with a fast commit log containing
EXT4_FC_TAG_ADD_RANGE for an inode lacking the EXT4_INODE_EXTENTS flag,
this would bypass the new check and could parse unvalidated indirect block
pointers as an extent header, potentially leading to out-of-bounds access
on EXT4_I(inode)->i_data.

> +		ret = ext4_ext_check(inode, eh, depth, 0);
> +		if (ret)
> +			goto err;
> +	}
>  
>  	if (path) {
>  		ext4_ext_drop_refs(path);

[Severity: High]
This is a pre-existing issue, but does ext4_find_extent() returning
ERR_PTR(-EFSCORRUPTED) lead to a panic in callers that check !path?

For example, in ext4_fc_replay_add_range():

fs/ext4/fast_commit.c:ext4_fc_replay_add_range() {
	...
			/* Range is not mapped */
			path = ext4_find_extent(inode, cur, NULL, 0);
			if (!path)
				continue;
	...
			ret = ext4_ext_insert_extent(
				NULL, inode, &path, &newex, 0);
	...
}

And similarly in ext4_ext_replay_update_ex():

fs/ext4/extents.c:ext4_ext_replay_update_ex() {
	...
	path = ext4_find_extent(inode, start, NULL, 0);
	if (!path)
		return -EINVAL;
	ex = path[path->p_depth].p_ext;
	...
}

Since an ERR_PTR is not NULL, the !path check evaluates to false, causing
the path error pointer to be dereferenced when passed to
ext4_ext_insert_extent() or when accessing path[path->p_depth].p_ext.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1