Re: [PATCH] ext4: fix out-of-bounds issue in ext4_xattr_set_entry

[Qianqiang Liu]

Hi Ojaswin,

On Tue, Oct 01, 2024 at 03:11:44PM +0530, Ojaswin Mujoo wrote:
> 
> Hey Qianqiang,
> 
> Thanks for the patch. I'm still reviewing this codepath but I do have
> some questions around the patch. So I understand that xattrs are
> arranged in the following format:
> 
>  *   +------------------+
>  *   | header           |
>  *   | entry 1          | 
>  *   | entry 2          | 
>  *   | entry 3          | 
>  *   | four null bytes  | <--- last
>  *   | . . .            | 
>  *   | . . .            | <--- here
>  *   | . . .            | 
>  *   | value 1          | 
>  *   | value 3          | 
>  *   | value 2          | 
>  *   +------------------+
> 
> Now, in this error, my understanding is that we are actually ending up
> in a case where "here" ie the place where the new xattr entry will go is
> beyond the "last" ie the last slot for xattr entry and that is causing
> an underflow, something like the above diagram.
> 
> My only concern is that why were we not able to detect this in the logic
> near the start of the function where we explcity check if we have enough
> space? 
> 
> Perhaps we should be fixing the logic in that if {..} instead
> since the comment a few lines above your fix:
> 
> 	/* No failures allowed past this point. */
> 
> does suggest that we can't error out below that point, so ideally all
> the checks would have been done before that.
> 
> I'm still going through the issue, will update here if needed.
> 
> Regards,
> ojaswin
> 

I reviewed the codepath, and here is the backtrace when the error occurs:

=> vfs_unlink
=> ext4_unlink
=> __ext4_unlink
=> __ext4_mark_inode_dirty
=> ext4_try_to_expand_extra_isize
=> __ext4_expand_extra_isize
=> ext4_expand_extra_isize_ea
=> ext4_xattr_make_inode_space
=> ext4_xattr_move_to_block -> ext4_xattr_block_find -> xattr_find_entry
=> ext4_xattr_block_set
=> ext4_xattr_set_entry
=> memmove((void *)here + size, here, rest);

The xattr_find_entry function return -ENODATA, but beacuase of the
following code, the error does not be returned to caller:

static int
ext4_xattr_block_find(struct inode *inode, struct ext4_xattr_info *i,
		      struct ext4_xattr_block_find *bs)
{
	...
	error = xattr_find_entry(inode, &bs->s.here, bs->s.end,
				 i->name_index, i->name, 1);
	if (error && error != -ENODATA)
		return error;
	...
}

So, perhaps we could modify the code as follows:

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index e0e1956dcdd3..649b278d4c1f 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -1884,7 +1884,7 @@ ext4_xattr_block_find(struct inode *inode, struct ext4_xattr_info *i,
 		bs->s.here = bs->s.first;
 		error = xattr_find_entry(inode, &bs->s.here, bs->s.end,
 					 i->name_index, i->name, 1);
-		if (error && error != -ENODATA)
+		if (error)
 			return error;
 		bs->s.not_found = error;
 	}

Or, we could check if s->not_found is -ENODATA in the ext4_xattr_set_entry function.

Any suggestions?

-- 
Best,
Qianqiang Liu