[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 at verbum.org>
>> Fixes: 431339ba9042 ("staging: erofs: add inode operations")
>> Signed-off-by: Gao Xiang <hsiangkao at linux.alibaba.com>
>> ---
>> 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