Re: [PATCH v2] erofs: fix incorrect symlink detection in fast symlink

[Colin Walters]

On Sun, Sep 8, 2024, at 11:19 PM, Gao Xiang wrote:
> Fast symlink can be used if the on-disk symlink data is stored
> in the same block as the on-disk inode, so we don’t need to trigger
> another I/O for symlink data.  However, correctly fs correction could be
> reported _incorrectly_ if inode xattrs are too large.
>
> In fact, these should be valid images although they cannot be handled as
> fast symlinks.
>
> Many thanks to Colin for reporting this!

Yes, though feel free to also add
Reported-by: https://honggfuzz.dev/ 
or so...not totally sure how to credit it "kernel style" bit honggfuzz very much helped me find this bug (indirectly).



>
> Reported-by: Colin Walters <walters@xxxxxxxxxx>
> Fixes: 431339ba9042 ("staging: erofs: add inode operations")
> Signed-off-by: Gao Xiang <hsiangkao@xxxxxxxxxxxxxxxxx>
> ---
> v2:
>  - sent out a wrong version (`m_pofs += vi->xattr_isize` was missed).
>
>  fs/erofs/inode.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
>
> diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
> index 5f6439a63af7..f2cab9e4f3bc 100644
> --- a/fs/erofs/inode.c
> +++ b/fs/erofs/inode.c
> @@ -178,12 +178,14 @@ static int erofs_fill_symlink(struct inode 
> *inode, void *kaddr,
>  			      unsigned int m_pofs)
>  {
>  	struct erofs_inode *vi = EROFS_I(inode);
> -	unsigned int bsz = i_blocksize(inode);
> +	loff_t off;
>  	char *lnk;
> 
> -	/* if it cannot be handled with fast symlink scheme */
> -	if (vi->datalayout != EROFS_INODE_FLAT_INLINE ||
> -	    inode->i_size >= bsz || inode->i_size < 0) {
> +	m_pofs += vi->xattr_isize;
> +	/* check if it cannot be handled with fast symlink scheme */
> +	if (vi->datalayout != EROFS_INODE_FLAT_INLINE || inode->i_size < 0 ||
> +	    check_add_overflow(m_pofs, inode->i_size, &off) ||
> +	    off > i_blocksize(inode)) {
>  		inode->i_op = &erofs_symlink_iops;
>  		return 0;
>  	}

This change LGTM.

> @@ -192,16 +194,6 @@ static int erofs_fill_symlink(struct inode *inode, 
> void *kaddr,
>  	if (!lnk)
>  		return -ENOMEM;
> 
> -	m_pofs += vi->xattr_isize;
> -	/* inline symlink data shouldn't cross block boundary */
> -	if (m_pofs + inode->i_size > bsz) {

It can cross a boundary but it still can't be *bigger* right? IOW do we still need to check here for inode->i_size > bsz or is that handled by something else generic?