linux-kernel - Re: [PATCH] ext4: xattr: fix wrong search.here in clone

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aUNbhrPEY9Aa2U4L@ndev>
Date: Thu, 18 Dec 2025 09:40:36 +0800
From: Jinchao Wang <wangjinchao600@...il.com>
To: Jan Kara <jack@...e.cz>
Cc: 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 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/

>When
> a search ends without success (-ENODATA error), s->here points to the
> 4-byte zeroed word inside xattr space that terminates the part with xattr
> headers. If I understand correctly the expression which overflows is:
> 
> size_t rest = (void *)last - (void *)here + sizeof(__u32);
> 
Yes, you are right about all of the above.

> 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:

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
    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