Message-ID: <qn25xtvqu76womw47qq6uhlhmudymvfqableicrodalzfkeo4r@qjwl5oegvlpd>
Date: Thu, 18 Dec 2025 16:39:08 +0100
From: Jan Kara <jack@...e.cz>
To: Jinchao Wang <wangjinchao600@...il.com>
Cc: Jan Kara <jack@...e.cz>, Theodore Ts'o <tytso@....edu>, 
	Andreas Dilger <adilger.kernel@...ger.ca>, linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org, 
	stable@...r.kernel.org, syzbot+f792df426ff0f5ceb8d1@...kaller.appspotmail.com
Subject: Re: [PATCH] ext4: xattr: fix wrong search.here in clone_block

On Thu 18-12-25 09:40:36, Jinchao Wang wrote:
> On Wed, Dec 17, 2025 at 12:30:15PM +0100, Jan Kara wrote:
> > Hello!
> > 
> > On Tue 16-12-25 19:34:55, Jinchao Wang wrote:
> > > syzbot reported a KASAN out-of-bounds Read in ext4_xattr_set_entry()[1].
> > > 
> > > When xattr_find_entry() returns -ENODATA, search.here still points to the
> > > position after the last valid entry. ext4_xattr_block_set() clones the xattr
> > > block because the original block maybe shared and must not be modified in
> > > place.
> > > 
> > > In the clone_block, search.here is recomputed unconditionally from the old
> > > offset, which may place it past search.first. This results in a negative
> > > reset size and an out-of-bounds memmove() in ext4_xattr_set_entry().
> > > 
> > > Fix this by initializing search.here correctly when search.not_found is set.
> > > 
> > > [1] https://syzkaller.appspot.com/bug?extid=f792df426ff0f5ceb8d1
> > > 
> > > Fixes: fd48e9acdf2 (ext4: Unindent codeblock in ext4_xattr_block_set)
> > > Cc: stable@...r.kernel.org
> > > Reported-by: syzbot+f792df426ff0f5ceb8d1@...kaller.appspotmail.com
> > > Signed-off-by: Jinchao Wang <wangjinchao600@...il.com>
> > 
> > Thanks for the patch! But I think the problem must be somewhere else. 
> The first syzbot test report was run without the patch applied,
> which caused confusion.
> The correct usage and report show that this patch fixes the crash:
> https://lore.kernel.org/all/20251216123945.391988-2-wangjinchao600@gmail.com/
> https://lore.kernel.org/all/6941580e.a70a0220.33cd7b.013d.GAE@google.com/

I was not arguing that your patch doesn't fix this syzbot issue. Just that
I don't understand how what you describe can happen and thus I'm not sure
whether the fix is really the best one...

> > in ext4_xattr_set_entry(). And I don't see how 'here' can be greater than
> > 'last' which should be pointing to the very same 4-byte zeroed word. The
> > fact that 'here' and 'last' are not equal is IMO the problem which needs
> > debugging and it indicates there's something really fishy going on with the
> > xattr block we work with. The block should be freshly allocated one as far
> > as I'm checking the disk image (as the 'file1' file doesn't have xattr
> > block in the original image).
> 
> I traced the crash path and find how this hapens:

Thanks for sharing the details!

> entry_SYSCALL_64
> ...
> ext4_xattr_move_to_block
>   ext4_xattr_block_find (){
>     error = xattr_find_entry(inode, &bs->s.here, ...); // bs->s.here updated 
>                                                        // to ENTRY(header(s->first)+1);
>     if (error && error != -ENODATA)
>       return error;
>     bs->s.not_found = error; // and returned to the caller
>   }
>   ext4_xattr_block_set (bs) {
>     s = bs->s;
>     offset = (char *)s->here - bs->bh->b_data; // bs->bh->b_data == bs->s.base
>                                                // offset = ENTRY(header(s->first)+1) - s.base
>                                                // leads to wrong offset

Why do you think the offset is wrong here? The offset is correct AFAICS -
it will be the offset of the 0 word from the beginning of xattr block. I
have run the reproducer myself and as I guessed in my previous email the
real problem is that someone modifies the xattr block between we compute
the offset here and the moment we call kmemdup() in clone_block. Thus the
computation of 'last' in ext4_xattr_set_entry() yields a different result
that what we saw in ext4_xattr_block_set(). The block modification happens
because the xattr block - block 33 is used for it - is also referenced from
file3 (but it was marked as unused in the block bitmap and so xattr block
got placed there).

So your patch was fixing the problem only by chance and slightly different
syzbot reproducer (overwriting the block 33 with a different contents)
would trigger the crash again.

So far I wasn't able to figure out how exactly the block 33 got zeroed out
but with corrupted filesystem it can happen in principle rather easily. The
question is how we can possibly fix this because this is one of the nastier
cases of fs corrution to deal with. The overhead of re-verifying fs
metadata each time we relock the buffer is just too big... So far no great
ideas for this.

								Honza

>     clone_block: {
> 	s->base = kmemdup(BHDR(bs->bh), bs->bh->b_size, GFP_NOFS);
> 	s->first = ENTRY(header(s->base)+1);
> 	s->here = ENTRY(s->base + offset); // wrong s->here
>     }
>     ext4_xattr_set_entry (s) {
>       last = s->first; // last < here
>       rest = (void *)last - (void *)here + sizeof(__u32); // negative rest
>       memmove((void *)here + size, here, rest); // crash
>     }
>   }
> > 
> > 								Honza
> > 
> > > diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> > > index 2e02efbddaac..cc30abeb7f30 100644
> > > --- a/fs/ext4/xattr.c
> > > +++ b/fs/ext4/xattr.c
> > > @@ -1980,7 +1980,10 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
> > >  			goto cleanup;
> > >  		s->first = ENTRY(header(s->base)+1);
> > >  		header(s->base)->h_refcount = cpu_to_le32(1);
> > > -		s->here = ENTRY(s->base + offset);
> > > +		if (s->not_found)
> > > +			s->here = s->first;
> > > +		else
> > > +			s->here = ENTRY(s->base + offset);
> > >  		s->end = s->base + bs->bh->b_size;
> > >  
> > >  		/*
> > > -- 
> > > 2.43.0
> > > 
> > -- 
> > Jan Kara <jack@...e.com>
> > SUSE Labs, CR
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR

Powered by blists - more mailing lists