Message-Id: <0093DAF4-7036-40EA-9051-082D3CD2115A@dilger.ca>
Date: Fri, 26 Sep 2025 13:47:14 -0600
From: Andreas Dilger <adilger@...ger.ca>
To: Deepanshu Kartikey <kartikey406@...il.com>
Cc: Theodore Ts'o <tytso@....edu>,
 linux-ext4 <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH v2] ext4: validate ea_ino and size in check_xattrs

On Sep 23, 2025, at 7:10 PM, Deepanshu Kartikey <kartikey406@...il.com> wrote:
> 
> Thank you for the feedback on the e2fsck coordination.
> 
> You raise a valid point about the complete repair workflow. I'm happy
> to work on a corresponding e2fsck patch if that would be helpful,
> though I'd appreciate guidance on the preferred approach:
> 
> 	• Should I proceed with the kernel patch first and then work
>           on e2fsck, or would you prefer coordinated patches?

Coordinated patches would be best.  My concern is that the check in
this patch is essentially causing the filesystem to be aborted, then
there may be no way to repair it before remount, repeat.

*NOTE* I haven't tested whether e2fsck already handles this scenario
correctly, but it is definitely worthwhile to test this with your
reproducer image to see if e2fsck already fixes the issue. If that is
already the case, then there is nothing more to be done.

A quick look through the users of e_value_inum in the e2fsck code
doesn't show anywhere that is checking e_value_size > 0, but the
e2fsprogs xattr  handling is spread over a few different parts of
the code so I might have missed something.

If e2fsck does *not* repair this error, then the right workflow is to
make a *minimal* filesystem image with this corruption and use it for
a new test case.  There is a "make testnew" target in tests/ that
creates a "f_testnew" subdirectory with an image file, then you can
mount this with loopback and/or use debugfs to create a large xattr
inode and zero e_value_size (not sure if in xattr header or the xattr
inode size, or maybe do some combinations of inconsistencies.

> 	• For the e2fsck side, would the appropriate fix be to:
> 		• Clear e_value_inum when e_value_size is zero, or
> 		• Remove the entire corrupted xattr entry?

Probably there is no benefit to remove e_value_inum and have an xattr without
any value, so it would be best to just remove this xattr completely.  The ext4
code should never (AFAIK) create a zero-length xattr in an external inode, as
this is totally pointless.

> I'm new to e2fsprogs development but willing to learn the codebase if you
> think it's valuable to have matching fixes. Alternatively, if there are
> others typically handling the e2fsck side of ext4 corruption fixes, I'm
> happy to focus on the kernel patch and coordinate with them.

The ext4 developers are typically handling both sides of the story together.

Cheers, Andreas

> Thanks for considering the broader user experience - I hadn't fully thought
> through the repair workflow.
> 
> 
> On Tue, Sep 23, 2025 at 11:34 PM Andreas Dilger <adilger@...ger.ca> wrote:
> On Sep 23, 2025, at 7:32 AM, Deepanshu Kartikey <kartikey406@...il.com> wrote:
> >
> > During xattr block validation, check_xattrs() processes xattr entries
> > without validating that entries claiming to use EA inodes have non-zero
> > sizes. Corrupted filesystems may contain xattr entries where e_value_size
> > is zero but e_value_inum is non-zero, indicating invalid xattr data.
> >
> > Add validation in check_xattrs() to detect this corruption pattern early
> > and return -EFSCORRUPTED, preventing invalid xattr entries from causing
> > issues throughout the ext4 codebase.
> 
> This should also have a corresponding check and fix in e2fsck, otherwise
> the kernel will fail but there is no way to repair such a filesystem.
> 
> Cheers, Andreas
> 
> > Suggested-by: Theodore Ts'o <tytso@....edu>
> > Reported-by: syzbot+4c9d23743a2409b80293@...kaller.appspotmail.com
> > Link: https://syzkaller.appspot.com/bug?extid=4c9d23743a2409b80293
> > Signed-off-by: Deepanshu Kartikey <kartikey406@...il.com>
> > ---
> > ---
> > Changes in v2:
> > - Moved validation from ext4_xattr_move_to_block() to check_xattrs() as suggested by Theodore Ts'o
> > - This provides broader coverage and may address other similar syzbot reports
> >
> > fs/ext4/xattr.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> > index 5a6fe1513fd2..d621e77c8c4d 100644
> > --- a/fs/ext4/xattr.c
> > +++ b/fs/ext4/xattr.c
> > @@ -251,6 +251,10 @@ check_xattrs(struct inode *inode, struct buffer_head *bh,
> >                       err_str = "invalid ea_ino";
> >                       goto errout;
> >               }
> > +             if (ea_ino && !size) {
> > +                     err_str = "invalid size in ea xattr";
> > +                     goto errout;
> > +             }
> >               if (size > EXT4_XATTR_SIZE_MAX) {
> >                       err_str = "e_value size too large";
> >                       goto errout;
> > --
> > 2.43.0
> >
> 
> 
> Cheers, Andreas
> 
> 
> 
> 
> 


Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)

Powered by blists - more mailing lists