Re: [PATCH] ext4: add bounds check for e_value_offs in ext4_read_inline_data

[Jan Kara]

On Fri 18-09-26 06:35:58, Deepanshu Kartikey wrote:
> ext4_read_inline_data() reads the location of an inline data xattr
> value directly from entry->e_value_offs without validating it against
> the actual bounds of the inode's xattr area. A corrupted filesystem
> image can set e_value_offs to an out-of-range value, causing the
> subsequent memcpy() to read from an address far outside the inode
> buffer, including memory that has already been freed and reused for
> something else. This mirrors the check already performed in
> ext4_xattr_ibody_get(), which is missing here.
> 
> Add a bounds check on the computed source pointer against the end of
> the inode's xattr area before the memcpy, and reject the read with
> -EFSCORRUPTED if it would go out of bounds.
> 
> Fixes: 67cf5b09a46f ("ext4: add the basic function for inline data support")
> Reported-by: syzbot+085a394c92518a04fd09@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=085a394c92518a04fd09
> Tested-by: syzbot+085a394c92518a04fd09@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Deepanshu Kartikey <kartikey406@xxxxxxxxx>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/ext4/inline.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> index ceee69a66482..2e60ab3e0db5 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
> @@ -187,6 +187,8 @@ static int ext4_read_inline_data(struct inode *inode, void *buffer,
>  	struct ext4_xattr_ibody_header *header;
>  	int cp_len = 0;
>  	struct ext4_inode *raw_inode;
> +	void *end, *p;
> +	u16 offset;
>  
>  	if (!len)
>  		return 0;
> @@ -205,13 +207,21 @@ static int ext4_read_inline_data(struct inode *inode, void *buffer,
>  		goto out;
>  
>  	header = IHDR(inode, raw_inode);
> +	end = ITAIL(inode, raw_inode);
>  	entry = (struct ext4_xattr_entry *)((void *)raw_inode +
>  					    EXT4_I(inode)->i_inline_off);
>  	len = min_t(unsigned int, len,
>  		    (unsigned int)le32_to_cpu(entry->e_value_size));
>  
> -	memcpy(buffer,
> -	       (void *)IFIRST(header) + le16_to_cpu(entry->e_value_offs), len);
> +	offset = le16_to_cpu(entry->e_value_offs);
> +	p = (void *)IFIRST(header) + offset;
> +
> +	if (unlikely(p + len > end)) {
> +		EXT4_ERROR_INODE(inode, "corrupt inline xattr entry");
> +		return -EFSCORRUPTED;
> +	}
> +
> +	memcpy(buffer, p, len);
>  	cp_len += len;
>  
>  out:
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR