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

[Gao Xiang <hsiangkao@linux.alibaba.com>]

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: stable@vger.kernel.org
> Fixes: 3aa8ec716e52 ("staging: erofs: add directory operations")
> Reported-by: Yuhao Jiang <danisjiang@gmail.com>
> Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
> ---
>   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.]

diff --git a/fs/erofs/dir.c b/fs/erofs/dir.c
index e5132575b9d3..e0666d6da9af 100644
--- a/fs/erofs/dir.c
+++ b/fs/erofs/dir.c
@@ -20,19 +20,18 @@ static int erofs_fill_dentries(struct inode *dir, struct dir_context *ctx,
  		unsigned int de_namelen;

  		/* the last dirent in the block? */
-		if (de + 1 >= end)
+		if (de + 1 >= end) {
+			if (maxsize <= nameoff)
+				goto err_bogus;
  			de_namelen = strnlen(de_name, maxsize - nameoff);
-		else
+		} else {
  			de_namelen = le16_to_cpu(de[1].nameoff) - 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;
-		}
+		    de_namelen > EROFS_NAME_LEN)
+			goto err_bogus;

  		if (!dir_emit(ctx, de_name, de_namelen,
  			      erofs_nid_to_ino64(EROFS_SB(dir->i_sb),
@@ -42,6 +41,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)


Thanks,
Gao Xiang

> +
>   		/* the last dirent in the block? */
>   		if (de + 1 >= end)
>   			de_namelen = strnlen(de_name, maxsize - nameoff);
> 
> ---
> base-commit: 7aaa8047eafd0bd628065b15757d9b48c5f9c07d
> change-id: 20260414-fixes-ae20cd389f52
> 
> Best regards,