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

[Gao Xiang]

On 2024/9/9 22:46, Colin Walters wrote:
> 
> 
> On Mon, Sep 9, 2024, at 10:14 AM, Gao Xiang wrote:
>>
>> Not quite sure about hard limitation in EROFS
>> runtime, we could define
>>
>> #define EROFS_SYMLINK_MAXLEN	4096
> 
> Not sure that a new define is needed versus just reusing PATH_MAX, but that's obviously just a style thing that's much more your call than mine.
> 
>> But since symlink i_size > 4096 only due to crafted
>> images (and not generated by mkfs) and not crash, so
>> either way (to check or not check in kernel) is okay
>> to me.
> 
> Yes, but my understanding was that EROFS (in contrast to other kernel read-write filesystems which are more complicated) was aiming to be robust against potentially malicious images.

Just my personal opinion, my understanding of rubustness
is stability and security.

But whether to check or not check this, it doesn't crash
the kernel or deadlock or livelock, so IMHO, it's already
rubustness.

Actually, I think EROFS for i_size > PAGE_SIZE, it's an
undefined or reserved behavior for now (just like CPU
reserved bits or don't care bits), just Linux
implementation treats it with PAGE_SIZE-1 trailing '\0',
but using erofs dump tool you could still dump large
symlinks.

Since PATH_MAX is a system-defined constant too, currently
Linux PATH_MAX is 4096, but how about other OSes? I've
seen some `PATH_MAX 8192` reference but I'm not sure which
OS uses this setting.

But I think it's a filesystem on-disk limitation, but if
i_size exceeds that, we return -EOPNOTSUPP or -EFSCORRUPTED?
For this symlink case, I tend to return -EFSCORRUPTED but
for other similar but complex cases, it could be hard to
decide.

Leaving them as undefined behaviors are also an option as
long as the behavior is secure.

> 
> Crafted/malicious images aside, there's also the IMO obvious angle here that we should avoid crashes or worse out-of-bound read/write if there happens to be *accidental* on-disk/memory corruption and having high bit(s) flip in a symlink inode size seems like an easy one to handle. Skimming the XFS code for example it looks like it's pretty robust in this area.

Yes, for this case it's much simple and easy so that's
fine, but I think for some other cases, leaving some
undefined or reserved behaviors are also good for later
extendability (again, like CPU register design.) as long
as it doesn't cause security issues.

Thanks,
Gao Xiang