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

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

[Yun Zhou]

Validate EA inode state in ext4_xattr_inode_iget() to prevent
WARN_ONCE triggers in ext4_xattr_inode_update_ref() and reject
corrupted EA inodes before they can cause further damage.

When a corrupted ext4 image has an EA inode with inconsistent i_nlink
and ref_count values, the code currently allows it through and later
hits WARN_ONCE when ref_count transitions cross the 0/1 boundary.
While this is not a security or stability issue -- it only fires on
crafted filesystem images and merely prints a call trace -- it 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>
---
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 | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 982a1f831e22..8efd6368f956 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -464,6 +464,33 @@ 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 corruption or
+	 * a concurrent last-reference drop.
+	 */
+	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));
+		/*
+		 * Mark rejected inode to prevent ext4_evict_inode() from
+		 * attempting truncation on a corrupted inode within an active
+		 * transaction, which could exhaust journal credits. The lock
+		 * serializes against ext4_xattr_inode_update_ref() which
+		 * does clear_nlink() + ext4_orphan_add() under the same lock.
+		 */
+		inode_lock_nested(inode, I_MUTEX_XATTR);
+		if (!ext4_inode_orphan_tracked(inode))
+			make_bad_inode(inode);
+		inode_unlock(inode);
+		iput(inode);
+		return -EFSCORRUPTED;
+	}
+
 	*ea_inode = inode;
 	return 0;
 }
-- 
2.43.0

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

[Jan Kara]

On Mon 15-06-26 13:35:12, Yun Zhou wrote:
> Validate EA inode state in ext4_xattr_inode_iget() to prevent
> WARN_ONCE triggers in ext4_xattr_inode_update_ref() and reject
> corrupted EA inodes before they can cause further damage.
> 
> When a corrupted ext4 image has an EA inode with inconsistent i_nlink
> and ref_count values, the code currently allows it through and later
> hits WARN_ONCE when ref_count transitions cross the 0/1 boundary.
> While this is not a security or stability issue -- it only fires on
> crafted filesystem images and merely prints a call trace -- it 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>
> ---
> 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 | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 982a1f831e22..8efd6368f956 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -464,6 +464,33 @@ 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 corruption or
> +	 * a concurrent last-reference drop.
> +	 */
> +	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));

Hum, given motivation of this is syzbot corrupted fs image, I'd just put
check in ext4_iget() verifying ext4_xattr_inode_get_ref() is > 0 and be
done with it. Much simpler and catches at least the obvious cases. The
consistency of xattr refcount and i_nlink is otherwise guarded by
ext4_xattr_inode_update_ref() and it can never be perfect as in the kernel
we don't have the full view of the filesystem and so cannot ascertain that
xattr ref count matches reality...

								Honza

> +		/*
> +		 * Mark rejected inode to prevent ext4_evict_inode() from
> +		 * attempting truncation on a corrupted inode within an active
> +		 * transaction, which could exhaust journal credits. The lock
> +		 * serializes against ext4_xattr_inode_update_ref() which
> +		 * does clear_nlink() + ext4_orphan_add() under the same lock.
> +		 */
> +		inode_lock_nested(inode, I_MUTEX_XATTR);
> +		if (!ext4_inode_orphan_tracked(inode))
> +			make_bad_inode(inode);
> +		inode_unlock(inode);
> +		iput(inode);
> +		return -EFSCORRUPTED;
> +	}
> +
>  	*ea_inode = inode;
>  	return 0;
>  }
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Zhou, Yun]

On 6/18/26 04:25, Jan Kara wrote:
> On Mon 15-06-26 13:35:12, Yun Zhou wrote:
>
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -464,6 +464,33 @@ 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 corruption or
> +      * a concurrent last-reference drop.
> +      */
> +     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));
> Hum, given motivation of this is syzbot corrupted fs image, I'd just put
> check in ext4_iget() verifying ext4_xattr_inode_get_ref() is > 0 and be
> done with it. Much simpler and catches at least the obvious cases. The
> consistency of xattr refcount and i_nlink is otherwise guarded by
> ext4_xattr_inode_update_ref() and it can never be perfect as in the kernel
> we don't have the full view of the filesystem and so cannot ascertain that
> xattr ref count matches reality...
>
Thanks very much for your review and suggestions, The issue should be easily
fixed after "introduce ext4_put_ea_inode() for safe deferred iput" 
merged. Just
need to defer iput() and no longer to make inode bad.

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

[Jan Kara]

On Thu 18-06-26 09:02:10, Zhou, Yun wrote:
> 
> 
> On 6/18/26 04:25, Jan Kara wrote:
> > On Mon 15-06-26 13:35:12, Yun Zhou wrote:
> > 
> > --- a/fs/ext4/xattr.c
> > +++ b/fs/ext4/xattr.c
> > @@ -464,6 +464,33 @@ 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 corruption or
> > +      * a concurrent last-reference drop.
> > +      */
> > +     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));
> > Hum, given motivation of this is syzbot corrupted fs image, I'd just put
> > check in ext4_iget() verifying ext4_xattr_inode_get_ref() is > 0 and be
> > done with it. Much simpler and catches at least the obvious cases. The
> > consistency of xattr refcount and i_nlink is otherwise guarded by
> > ext4_xattr_inode_update_ref() and it can never be perfect as in the kernel
> > we don't have the full view of the filesystem and so cannot ascertain that
> > xattr ref count matches reality...
> > 
> Thanks very much for your review and suggestions, The issue should be easily
> fixed after "introduce ext4_put_ea_inode() for safe deferred iput" merged.
> Just need to defer iput() and no longer to make inode bad.

OK, just to make it clear, calling make_bad_inode() is never a right thing
to do once the inode has been exposed to other users. It is only usable
within iget functions when loading inode into memory and finding out the
content isn't valid. This function is one of the traps we have in VFS...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR