Re: [PATCH v3] erofs: fix the out-of-bounds nameoff handling for trailing dirents

On 2026/4/21 15:26, Chao Yu wrote:
> On 4/16/2026 5:44 PM, Gao Xiang wrote:
>> Currently we already have boundary-checks for nameoffs, but the trailing
>> dirents are special since the namelens are calculated with strnlen()
>> with unchecked nameoffs.
>>
>> If a crafted EROFS has a trailing dirent with nameoff >= maxsize,
>> maxsize - nameoff can underflow, causing strnlen() to read past the
>> directory block.
>>
>> nameoff0 should also be verified to be a multiple of
>> `sizeof(struct erofs_dirent)` as well [1].
>>
>> [1] https://sashiko.dev/#/patchset/20260416063511.3173774-1-hsiangkao%40linux.alibaba.com
>> Fixes: 3aa8ec716e52 ("staging: erofs: add directory operations")
>> Fixes: 33bac912840f ("staging: erofs: keep corrupted fs from crashing kernel in erofs_readdir()")
>> Reported-by: Yuhao Jiang <[email protected]>
>> Reported-by: Junrui Luo <[email protected]>
>> Closes: https://lore.kernel.org/r/[email protected]
>> Cc: [email protected]
>> Signed-off-by: Gao Xiang <[email protected]>
>> ---
>> v3:
>>   - Disallow unaligned nameoff0 to avoid petential oob reads as well.
>>
>>   fs/erofs/dir.c | 29 ++++++++++++++++-------------
>>   1 file changed, 16 insertions(+), 13 deletions(-)
>>
>> diff --git a/fs/erofs/dir.c b/fs/erofs/dir.c
>> index e5132575b9d3..d074fded1577 100644
>> --- a/fs/erofs/dir.c
>> +++ b/fs/erofs/dir.c
>> @@ -19,20 +19,18 @@ static int erofs_fill_dentries(struct inode *dir, struct dir_context *ctx,
>>           const char *de_name = (char *)dentry_blk + nameoff;
>>           unsigned int de_namelen;
>> -        /* the last dirent in the block? */
>> -        if (de + 1 >= end)
>> -            de_namelen = strnlen(de_name, maxsize - nameoff);
>> -        else
>> +        /* non-trailing dirent in the directory block? */
>> +        if (de + 1 < end)
>>               de_namelen = le16_to_cpu(de[1].nameoff) - nameoff;
>> +        else if (maxsize <= nameoff)
>> +            goto err_bogus;
>> +        else
>> +            de_namelen = strnlen(de_name, maxsize - nameoff);
>> -        /* a corrupted entry is found */
>> -        if (nameoff + de_namelen > maxsize ||
>> -            de_namelen > EROFS_NAME_LEN) {
>> -            erofs_err(dir->i_sb, "bogus dirent @ nid %llu",
>> -                  EROFS_I(dir)->nid);
>> -            DBG_BUGON(1);
>> -            return -EFSCORRUPTED;
>> -        }
>> +        /* a corrupted entry is found (including negative namelen) */
>> +        if (!in_range32(de_namelen, 1, EROFS_NAME_LEN) ||
>> +            nameoff + de_namelen > maxsize)
>> +            goto err_bogus;
>>           if (!dir_emit(ctx, de_name, de_namelen,
>>                     erofs_nid_to_ino64(EROFS_SB(dir->i_sb),
>> @@ -42,6 +40,10 @@ static int erofs_fill_dentries(struct inode *dir, struct dir_context *ctx,
>>           ctx->pos += sizeof(struct erofs_dirent);
>>       }
>>       return 0;
>> +err_bogus:
>> +    erofs_err(dir->i_sb, "bogus dirent @ nid %llu", EROFS_I(dir)->nid);
>> +    DBG_BUGON(1);
>> +    return -EFSCORRUPTED;
>>   }
>>   static int erofs_readdir(struct file *f, struct dir_context *ctx)
>> @@ -88,7 +90,8 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
>>           }
>>           nameoff = le16_to_cpu(de->nameoff);
>> -        if (nameoff < sizeof(struct erofs_dirent) || nameoff >= bsz) {
> 
> You mean?
> 
> if (!nameoff || nameoff >= bsz || nameoff % sizeof(struct erofs_dirent))

The explanation can be seen as:
https://sashiko.dev/#/patchset/20260416063511.3173774-1-hsiangkao%40linux.alibaba.com

But I think `nameoff < sizeof(struct erofs_dirent)` is also fine?
I could also switch to your suggested version.

Thanks,
Gao Xiang