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

[Gao Xiang]

Hi Colin,

On 2024/9/9 20:48, Colin Walters wrote:
> 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).

Ok, it sounds good to me.
I will add this later.

> > 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?

It can be bigger.

Like ext4, EROFS supports PAGE_SIZE symlink via page_get_link()
(non-fastsymlink cases), but mostly consider this as 4KiB though
regardless of on-disk block sizes.

Thanks,
Gao Xiang