Re: [PATCH] erofs: validate nameoff for all dirents in erofs_fill_dentries()

On 2026/4/15 00:00, Gao Xiang wrote:
> Hi Junrui,
> 
> On 2026/4/14 23:20, Junrui Luo wrote:
>> erofs_readdir() validates de[0].nameoff before calling
>> erofs_fill_dentries(), but subsequent dirents are used without
>> validation. The loop computes `maxsize - nameoff` as an unsigned int
>> to bound strnlen().
> 
> The issue is true, but I don't think the description is valid.
> 
> I think what we missed is to check the last dirent nameoff vs
> maxsize.
> 
> BTW, please don't "To" too many people (especially Miao Xie
> and Greg), basically I think you only need to post to people
> according to `./checkpoint.pl` but leave indivudual person
> into "Cc" instead.
> 
>>
>> If a crafted EROFS image has a dirent with nameoff >= maxsize, the
>> subtraction underflows, causing strnlen() to read past the block
>> buffer.
>>
>> Fix by validating each entry's nameoff at the top of the loop: it
>> must be >= nameoff0 and <= maxsize.
>>
>> Cc: [email protected]
>> Fixes: 3aa8ec716e52 ("staging: erofs: add directory operations")
>> Reported-by: Yuhao Jiang <[email protected]>
>> Signed-off-by: Junrui Luo <[email protected]>
>> ---
>>   fs/erofs/dir.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/fs/erofs/dir.c b/fs/erofs/dir.c
>> index e5132575b9d3..2efa16fa162f 100644
>> --- a/fs/erofs/dir.c
>> +++ b/fs/erofs/dir.c
>> @@ -19,6 +19,13 @@ static int erofs_fill_dentries(struct inode *dir, struct dir_context *ctx,
>>           const char *de_name = (char *)dentry_blk + nameoff;
>>           unsigned int de_namelen;
>> +        if (nameoff < nameoff0 || nameoff > maxsize) {
>> +            erofs_err(dir->i_sb, "bogus dirent @ nid %llu",
>> +                  EROFS_I(dir)->nid);
>> +            DBG_BUGON(1);
>> +            return -EFSCORRUPTED;
>> +        }
> 
> I think the only thing we need is the following diff:
> 
> [The reason why nameoff < nameoff0 is unneeded, since
>   `de_namelen > EROFS_NAME_LEN` ensures the nameoff delta
>   won't be negative (so nameoff will increase.)
> 
>   and `nameoff + de_namelen > maxsize` implies
>   `nameoff > maxsize` so `nameoff > maxsize` is unneeded too.]

A even better diff is as below:

diff --git a/fs/erofs/dir.c b/fs/erofs/dir.c
index e5132575b9d3..2b8375c7d523 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;
-		}
+		if (!clamp(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)