[PATCH v5] ext4: validate EA inode i_nlink in ext4_xattr_inode_iget

[PATCH v5] ext4: validate EA inode i_nlink in ext4_xattr_inode_iget

[Yun Zhou]

Validate EA inode state in ext4_xattr_inode_iget() to reject corrupted
EA inodes early, before they trigger WARN_ONCE in
ext4_xattr_inode_update_ref().

When a corrupted ext4 image has an EA inode with inconsistent i_nlink
and ref_count values (e.g. i_nlink=65535), the code currently allows it
through and later hits WARN_ONCE when ref_count transitions cross the
0/1 boundary.  This is better handled as an early sanity check that
returns -EFSCORRUPTED, consistent with how ext4 treats other on-disk
corruption.

Since ext4_xattr_inode_iget() resolves references from active xattr
entries, the target EA inode must be in active state (i_nlink=1,
ref_count>0).  Reject any inode that does not satisfy this.

Reported-by: syzbot+76916a45d2294b551fd9@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=76916a45d2294b551fd9
Fixes: dec214d00e0d ("ext4: xattr inode deduplication")
Signed-off-by: Yun Zhou <yun.zhou@windriver.com>
---
v5:
 - Use ext4_put_ea_inode() instead of make_bad_inode() + iput() to
   safely release the rejected inode regardless of jbd2 handle or
   SB_ACTIVE state.

v4:
 - Take I_MUTEX_XATTR before checking orphan state to safely decide
   whether to call make_bad_inode(), avoiding orphan list corruption
   if another thread is concurrently freeing the EA inode

v3:
 - Move check after Lustre branch to avoid false positives on Lustre EA inodes
 - Merge into single condition: i_nlink != 1 || !ref_count
 - Add make_bad_inode() before iput() to avoid truncation in active txn

v2:
 - Add ref_count validation to also catch i_nlink=1, ref_count=0 case

 fs/ext4/xattr.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 4ae6ce111566..1eee25955107 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -460,6 +460,21 @@ static int ext4_xattr_inode_iget(struct inode *parent, unsigned long ea_ino,
 		inode_unlock(inode);
 	}
 
+	/*
+	 * Since this function resolves references from active xattr entries,
+	 * the EA inode must be in active state (i_nlink=1, ref_count>0).
+	 * i_nlink > 1, i_nlink == 0 (dangling reference), or ref_count == 0
+	 * (inconsistent with an active entry) all indicate on-disk corruption.
+	 */
+	if (inode->i_nlink != 1 || !ext4_xattr_inode_get_ref(inode)) {
+		ext4_error(parent->i_sb,
+			   "EA inode %lu has unexpected i_nlink=%u ref_count=%llu",
+			   ea_ino, inode->i_nlink,
+			   ext4_xattr_inode_get_ref(inode));
+		ext4_put_ea_inode(inode);
+		return -EFSCORRUPTED;
+	}
+
 	*ea_inode = inode;
 	return 0;
 }
-- 
2.43.0

Re: [PATCH v5] ext4: validate EA inode i_nlink in ext4_xattr_inode_iget

[Jan Kara]

On Fri 24-07-26 18:02:55, Yun Zhou wrote:
> Validate EA inode state in ext4_xattr_inode_iget() to reject corrupted
> EA inodes early, before they trigger WARN_ONCE in
> ext4_xattr_inode_update_ref().
> 
> When a corrupted ext4 image has an EA inode with inconsistent i_nlink
> and ref_count values (e.g. i_nlink=65535), the code currently allows it
> through and later hits WARN_ONCE when ref_count transitions cross the
> 0/1 boundary.  This is better handled as an early sanity check that
> returns -EFSCORRUPTED, consistent with how ext4 treats other on-disk
> corruption.
> 
> Since ext4_xattr_inode_iget() resolves references from active xattr
> entries, the target EA inode must be in active state (i_nlink=1,
> ref_count>0).  Reject any inode that does not satisfy this.
> 
> Reported-by: syzbot+76916a45d2294b551fd9@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=76916a45d2294b551fd9
> Fixes: dec214d00e0d ("ext4: xattr inode deduplication")
> Signed-off-by: Yun Zhou <yun.zhou@windriver.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
> v5:
>  - Use ext4_put_ea_inode() instead of make_bad_inode() + iput() to
>    safely release the rejected inode regardless of jbd2 handle or
>    SB_ACTIVE state.
> 
> v4:
>  - Take I_MUTEX_XATTR before checking orphan state to safely decide
>    whether to call make_bad_inode(), avoiding orphan list corruption
>    if another thread is concurrently freeing the EA inode
> 
> v3:
>  - Move check after Lustre branch to avoid false positives on Lustre EA inodes
>  - Merge into single condition: i_nlink != 1 || !ref_count
>  - Add make_bad_inode() before iput() to avoid truncation in active txn
> 
> v2:
>  - Add ref_count validation to also catch i_nlink=1, ref_count=0 case
> 
>  fs/ext4/xattr.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 4ae6ce111566..1eee25955107 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -460,6 +460,21 @@ static int ext4_xattr_inode_iget(struct inode *parent, unsigned long ea_ino,
>  		inode_unlock(inode);
>  	}
>  
> +	/*
> +	 * Since this function resolves references from active xattr entries,
> +	 * the EA inode must be in active state (i_nlink=1, ref_count>0).
> +	 * i_nlink > 1, i_nlink == 0 (dangling reference), or ref_count == 0
> +	 * (inconsistent with an active entry) all indicate on-disk corruption.
> +	 */
> +	if (inode->i_nlink != 1 || !ext4_xattr_inode_get_ref(inode)) {
> +		ext4_error(parent->i_sb,
> +			   "EA inode %lu has unexpected i_nlink=%u ref_count=%llu",
> +			   ea_ino, inode->i_nlink,
> +			   ext4_xattr_inode_get_ref(inode));
> +		ext4_put_ea_inode(inode);
> +		return -EFSCORRUPTED;
> +	}
> +
>  	*ea_inode = inode;
>  	return 0;
>  }
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR