Re: [PATCH] ext4: prevent out-of-bounds read in ext4_read_inline_data()

[Jan Kara]

On Tue 21-04-26 17:31:38, Junjie Cao wrote:
> ext4_read_inline_data() reads e_value_offs from the inode buffer_head on
> each call, but the decision to enter the xattr value path depends on
> i_inline_size cached in EXT4_I(inode) at iget time. If the buffer
> contents change after the initial validation, e_value_offs can point
> beyond the inode body while i_inline_size still directs the code into
> the xattr value path, causing an out-of-bounds read in the memcpy.
> 
> Add a bounds check before the memcpy, consistent with
> ext4_xattr_ibody_get(). Also guard folio_mark_uptodate() in
> ext4_read_inline_folio() since ext4_read_inline_data() can now return
> -EFSCORRUPTED.
> 
> Fixes: 67cf5b09a46f ("ext4: add the basic function for inline data support")
> Cc: stable@xxxxxxxxxxxxxxx
> Reported-by: syzbot+26c4a8cab92d0cda3e3b@xxxxxxxxxxxxxxxxxxxxxxxxx
> Tested-by: syzbot+26c4a8cab92d0cda3e3b@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=26c4a8cab92d0cda3e3b
> Signed-off-by: Junjie Cao <junjie.cao@xxxxxxxxx>

If the buffer contents changes after the initial validation, there is some
problem somewhere and this isn't going to fix it (likely the fs is
corrupted and that isn't properly detected). Please fix the real problem,
not just paper over it.

								Honza

> ---
>  fs/ext4/inline.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> index 408677fa8196..18c678df0a6e 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
> @@ -211,6 +211,14 @@ static int ext4_read_inline_data(struct inode *inode, void *buffer,
>  	len = min_t(unsigned int, len,
>  		    (unsigned int)le32_to_cpu(entry->e_value_size));
>  
> +	if (unlikely((void *)IFIRST(header) + le16_to_cpu(entry->e_value_offs) +
> +		     len > (void *)ITAIL(inode, raw_inode))) {
> +		EXT4_ERROR_INODE(inode,
> +			"inline data value out of bounds (offs %u len %u)",
> +			le16_to_cpu(entry->e_value_offs), len);
> +		return -EFSCORRUPTED;
> +	}
> +
>  	memcpy(buffer,
>  	       (void *)IFIRST(header) + le16_to_cpu(entry->e_value_offs), len);
>  	cp_len += len;
> @@ -535,7 +543,8 @@ static int ext4_read_inline_folio(struct inode *inode, struct folio *folio)
>  	ret = ext4_read_inline_data(inode, kaddr, len, &iloc);
>  	kaddr = folio_zero_tail(folio, len, kaddr + len);
>  	kunmap_local(kaddr);
> -	folio_mark_uptodate(folio);
> +	if (ret >= 0)
> +		folio_mark_uptodate(folio);
>  	brelse(iloc.bh);
>  
>  out:
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR