Re: [PATCH] ext4: validate extent entries before caching in ext4_find_extent()

Re: [PATCH] ext4: validate extent entries before caching in ext4_find_extent()

[Deepanshu Kartikey]

Zhang Yi,

You're correct that ext4_ext_check_inode() should catch this. I investigated and found why it doesn't:
[   18.824142] DEBUG: Validating inode 2 (no inline data)
[   18.835777] DEBUG: verity inode 15, inline=0, extents=1
[   18.836793] DEBUG: Skipping validation for inode 15 (has inline data)

The verity inode reports inline=0 when checking the flag directly, but ext4_has_inline_data() returns true at the validation check, causing validation to be skipped.

This corrupted filesystem has a verity file that somehow triggers the inline data check to return true, even though verity files should never have inline data. This allows the corrupted out-of-order extents to bypass validation.

My patch adds validation before caching extents, which catches such corruption regardless of why the inline data check fails. This provides necessary defense against corrupted filesystems at the point where extents are actually used.

Re: [PATCH] ext4: validate extent entries before caching in ext4_find_extent()

[Zhang Yi]

On 9/29/2025 1:27 PM, Deepanshu Kartikey wrote:
> Zhang Yi,
> 
> You're correct that ext4_ext_check_inode() should catch this. I investigated and found why it doesn't:
> [   18.824142] DEBUG: Validating inode 2 (no inline data)
> [   18.835777] DEBUG: verity inode 15, inline=0, extents=1
> [   18.836793] DEBUG: Skipping validation for inode 15 (has inline data)
> 
> The verity inode reports inline=0 when checking the flag directly, but ext4_has_inline_data() returns true at the validation check, causing validation to be skipped.

This makes me confused, how can this inode report inline=0 when checking
the flag directly, but ext4_has_inline_data() returns true?

Does this mean that this inode has both the EXT4_INODE_EXTENTS and
EXT4_INODE_INLINE_DATA flags set? If so, we should detect this in
ext4_iget() and call ext4_error_inode().

> 
> This corrupted filesystem has a verity file that somehow triggers the inline data check to return true, even though verity files should never have inline data. This allows the corrupted out-of-order extents to bypass validation.
> 
> My patch adds validation before caching extents, which catches such corruption regardless of why the inline data check fails. This provides necessary defense against corrupted filesystems at the point where extents are actually used.

Generally speaking, we should avoid redundant checks. It is sufficient to
verify the metadata after reading it from the disk, without considering
scenarios which intentionally corrupted the metadata by directly writing
to the bdev. Adding checks in ext4_find_extent() would introduce
unnecessary overhead.

Thanks,
Yi.

Re: [PATCH] ext4: validate extent entries before caching in ext4_find_extent()

[Deepanshu Kartikey]

Zhang Yi,

I've prepared v2 based on your feedback about avoiding redundant checks. 
After further analysis, I'm detecting the invalid INLINE_DATA + EXTENTS 
flag combination rather than just VERITY + INLINE_DATA, as this addresses 
the broader issue of mutually exclusive flags.

The v2 patch rejects any inode with both INLINE_DATA and EXTENTS flags set,
which catches this corruption and potentially other similar cases.

Thank you for guiding me to the root cause.

Best regards,
Deepanshu

Re: [PATCH] ext4: validate extent entries before caching in ext4_find_extent()

[Deepanshu Kartikey]

Zhang Yi,

Thank you for pointing out the validation issue and your concerns about redundant checks. Let me provide a complete explanation of what's happening.

## Initial Problem
The reproducer triggers a BUG_ON in ext4_es_cache_extent() when opening a verity file on a corrupted ext4 filesystem. The issue occurs because the extent tree contains out-of-order extents that cause integer underflow when calculating hole sizes.

## Why ext4_ext_check_inode() Doesn't Catch This
You correctly asked why the existing validation in __ext4_iget() doesn't catch this corruption. After investigation with debug code, I found:

DEBUG: verity inode 15, inline=0, extents=1, test_inode_flag_inline_data=1
DEBUG: First time check inode 15 - flag=1, i_inline_off=0, has_inline=0  
DEBUG: Second time check inode 15 - flag=1, i_inline_off=164, has_inline=1
DEBUG: Skipping validation for inode 15 (has inline data)

The corrupted filesystem has inode 15 with:
1. EXT4_INODE_INLINE_DATA flag set (test_inode_flag returns 1)
2. EXT4_INODE_VERITY flag set (it's a verity file)
3. i_inline_off containing value 164 (from corrupted on-disk data)
4. Out-of-order extents in the extent tree

## The Validation Bypass Mechanism
The validation code in __ext4_iget():
} else if (!ext4_has_inline_data(inode)) {
    /* validate the block references in the inode */

The ext4_has_inline_data() function returns:
return ext4_test_inode_flag(inode, EXT4_INODE_INLINE_DATA) && 
       EXT4_I(inode)->i_inline_off;

Initially i_inline_off=0, so ext4_has_inline_data() returns false (1 && 0 = 0). But by the time validation check happens, i_inline_off=164 (loaded from corrupted on-disk data), making ext4_has_inline_data() return true (1 && 164 = 1), which skips validation.

## Proper Solution
You're correct that we should avoid redundant checks. The proper fix is to detect this invalid combination early in ext4_iget():

if (ext4_test_inode_flag(inode, EXT4_INODE_VERITY) &&
    ext4_test_inode_flag(inode, EXT4_INODE_INLINE_DATA)) {
    ext4_error_inode(inode, __func__, __LINE__, 0,
                     "inode has both verity and inline data flags");
    ret = -EFSCORRUPTED;
    goto bad_inode;
}

This addresses the root cause without adding overhead to the extent lookup path. I'll prepare a v2 patch implementing this approach instead of adding validation in ext4_find_extent().

Thank you for the thorough review that led to finding the actual root cause.

Best regards,
Deepanshu

Re: [PATCH] ext4: validate extent entries before caching in ext4_find_extent()

[Deepanshu Kartikey]

[PATCH] ext4: validate extent entries before caching in ext4_find_extent()

[Deepanshu Kartikey]

syzbot reported a BUG_ON in ext4_es_cache_extent() triggered when
opening a verity file on a corrupted ext4 filesystem mounted without
a journal.

The issue occurs when the extent tree contains out-of-order extents,
which can happen in a corrupted filesystem. ext4_find_extent() calls
ext4_cache_extents() without validating the extent entries when the
tree depth is 0 (leaf level). This allows corrupted extent trees with
out-of-order extents to be cached, triggering a BUG_ON in
ext4_es_cache_extent() due to integer underflow when calculating hole
sizes:

  If prev = 4352 and lblk = 1280:
  lblk - prev = 1280 - 4352 = -3072 (as signed)
  = 4294964224 (as unsigned)
  end = lblk + len - 1 = 4352 + 4294964224 - 1 = 1279 (after overflow)
  BUG_ON(end < lblk) triggers because 1279 < 4352

Fix this by adding extent entry validation using the existing
ext4_valid_extent_entries() function before caching. This ensures
corrupted extent trees are detected and handled properly through the
error path, preventing both the BUG_ON and potential use-after-free
issues.

Reported-and-tested-by: syzbot+038b7bf43423e132b308@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=038b7bf43423e132b308
Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
---
 fs/ext4/extents.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index ca5499e9412b..f8e45623f7ea 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -924,8 +924,18 @@ ext4_find_extent(struct inode *inode, ext4_lblk_t block,
 	path[0].p_bh = NULL;
 
 	i = depth;
-	if (!(flags & EXT4_EX_NOCACHE) && depth == 0)
+	if (!(flags & EXT4_EX_NOCACHE) && depth == 0) {
+		ext4_fsblk_t pblk = 0;
+
+		if (!ext4_valid_extent_entries(inode, eh, 0, &pblk, 0)) {
+			EXT4_ERROR_INODE(inode,
+				"invalid extent entries, pblk %llu",
+				pblk);
+			ret = -EFSCORRUPTED;
+			goto err;
+		}
 		ext4_cache_extents(inode, eh);
+	}
 	/* walk through the tree */
 	while (i) {
 		ext_debug(inode, "depth %d: num %d, max %d\n",
-- 
2.43.0

Re: [PATCH] ext4: validate extent entries before caching in ext4_find_extent()

[Zhang Yi]

Hi, Deepanshu!

On 9/28/2025 6:09 PM, Deepanshu Kartikey wrote:
> syzbot reported a BUG_ON in ext4_es_cache_extent() triggered when
> opening a verity file on a corrupted ext4 filesystem mounted without
> a journal.
> 
> The issue occurs when the extent tree contains out-of-order extents,
> which can happen in a corrupted filesystem. ext4_find_extent() calls
> ext4_cache_extents() without validating the extent entries when the
> tree depth is 0 (leaf level). This allows corrupted extent trees with
> out-of-order extents to be cached, triggering a BUG_ON in
> ext4_es_cache_extent() due to integer underflow when calculating hole
> sizes:
> 
>   If prev = 4352 and lblk = 1280:
>   lblk - prev = 1280 - 4352 = -3072 (as signed)
>   = 4294964224 (as unsigned)
>   end = lblk + len - 1 = 4352 + 4294964224 - 1 = 1279 (after overflow)
>   BUG_ON(end < lblk) triggers because 1279 < 4352
> 
> Fix this by adding extent entry validation using the existing
> ext4_valid_extent_entries() function before caching. This ensures
> corrupted extent trees are detected and handled properly through the
> error path, preventing both the BUG_ON and potential use-after-free
> issues.
> 

Thank you for the fix patch! However, I am curious why the check in
__ext4_iget()->ext4_ext_check_inode() fails? It should check the
extents of the root node and be able to caught this corruption.

Thanks,
Yi

> Reported-and-tested-by: syzbot+038b7bf43423e132b308@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=038b7bf43423e132b308
> Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
> ---
>  fs/ext4/extents.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index ca5499e9412b..f8e45623f7ea 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -924,8 +924,18 @@ ext4_find_extent(struct inode *inode, ext4_lblk_t block,
>  	path[0].p_bh = NULL;
>  
>  	i = depth;
> -	if (!(flags & EXT4_EX_NOCACHE) && depth == 0)
> +	if (!(flags & EXT4_EX_NOCACHE) && depth == 0) {
> +		ext4_fsblk_t pblk = 0;
> +
> +		if (!ext4_valid_extent_entries(inode, eh, 0, &pblk, 0)) {
> +			EXT4_ERROR_INODE(inode,
> +				"invalid extent entries, pblk %llu",
> +				pblk);
> +			ret = -EFSCORRUPTED;
> +			goto err;
> +		}
>  		ext4_cache_extents(inode, eh);
> +	}
>  	/* walk through the tree */
>  	while (i) {
>  		ext_debug(inode, "depth %d: num %d, max %d\n",