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. 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);
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).
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;elses->here = ENTRY(s->base + offset);/* -- 2.43.0