Re: [PATCH] ext4: xattr: fix wrong search.here in clone_block

[Jinchao Wang <wangjinchao600@gmail.com>]

On Thu, Dec 18, 2025 at 04:39:08PM +0100, Jan Kara wrote:
> 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@vger.kernel.org
> > > > Reported-by: syzbot+f792df426ff0f5ceb8d1@syzkaller.appspotmail.com
> > > > Signed-off-by: Jinchao Wang <wangjinchao600@gmail.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
> 

Baokun explained part of the process in the kernel space.
https://lore.kernel.org/all/d62a25e9-04de-4309-98d1-22a4f9b5bb49@huawei.com/

I analysed syz-reproducer and add some userspace details:

- original filesystem state
  - file1:
    - inode 15 with File ACL block 33
  - file2:
    - inode 16 with data blocks 27–35
- actions
  - syscall(__NR_creat, "file2")
  - syscall(__NR_unlink, "file1")  // panic happens here

The original filesystem state is already corrupted, with block 33 beging
referenced both as an xattr block and as file data.