romfs: reject directory entry chains that do not move forward

[PATCH RESEND] romfs: reject directory entry chains that do not move forward

Posted by Palla Raghunath 6 days, 6 hours ago

romfs_lookup() and romfs_readdir() walk a directory's entry chain by
following ri.next, and stop only once the offset is zero or past the end
of the image. Nothing stops an entry from pointing at itself, so on a
crafted image a lookup of a name that is not present spins forever.

That loop has no cond_resched() and no signal check, so the task cannot
be killed, and it spins with i_rwsem held and the dentry left in the
in-lookup state. A second lookup of the same name then waits forever in
d_alloc_parallel(), and it is that waiter the hung task watchdog
reports, which makes this look like a dcache problem:

  INFO: task syz-executor239:5636 blocked for more than 143 seconds.
   d_alloc_parallel+0xd13/0x16c0 fs/dcache.c:2839
   __lookup_slow+0x82/0x2f0 fs/namei.c:1904

Require the chain to move forward and return -EIO if it does not. File
headers are laid out in increasing order and both loops already treat
the chain as a bounded forward-only walk, so this only enforces what
they assume. In romfs_readdir() a cycle never advances ctx->pos and has
getdents() repeat entries forever, so check there too.

The hard link chain bounded by commit d30b5a954e0a ("romfs: detect hard
link cycles") is a separate one. Sangho Lee posted a fix for this chain
in July 2026 that validates it at inode instantiation; it has had no
replies.

Reported-by: syzbot+e8ebfecec06a009dd2d3@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=e8ebfecec06a009dd2d3
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Palla Raghunath <raghunathpalla.0209@gmail.com>
---
 fs/romfs/super.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/fs/romfs/super.c b/fs/romfs/super.c
index 7f783d6173e8..846b57f265ef 100644
--- a/fs/romfs/super.c
+++ b/fs/romfs/super.c
@@ -143,7 +143,7 @@ static int romfs_readdir(struct file *file, struct dir_context *ctx)
 {
 	struct inode *i = file_inode(file);
 	struct romfs_inode ri;
-	unsigned long offset, maxoff;
+	unsigned long offset, maxoff, next;
 	int j, ino, nextfh;
 	char fsname[ROMFS_MAXFN];	/* XXX dynamic? */
 	int ret;
@@ -191,7 +191,16 @@ static int romfs_readdir(struct file *file, struct dir_context *ctx)
 			    romfs_dtype_table[nextfh & ROMFH_TYPE]))
 			goto out;
 
-		offset = nextfh & ROMFH_MASK;
+		/* likewise, a chain that does not move forward would have
+		 * getdents() repeat entries forever
+		 */
+		next = nextfh & ROMFH_MASK;
+		if (next && next <= offset) {
+			offset = maxoff;
+			ctx->pos = offset;
+			goto out;
+		}
+		offset = next;
 	}
 out:
 	return 0;
@@ -203,7 +212,7 @@ static int romfs_readdir(struct file *file, struct dir_context *ctx)
 static struct dentry *romfs_lookup(struct inode *dir, struct dentry *dentry,
 				   unsigned int flags)
 {
-	unsigned long offset, maxoff;
+	unsigned long offset, maxoff, next;
 	struct inode *inode = NULL;
 	struct romfs_inode ri;
 	const char *name;		/* got from dentry */
@@ -245,8 +254,15 @@ static struct dentry *romfs_lookup(struct inode *dir, struct dentry *dentry,
 			break;
 		}
 
-		/* next entry */
-		offset = be32_to_cpu(ri.next) & ROMFH_MASK;
+		/* next entry; the chain must move forward, or a corrupt image
+		 * will keep us here forever
+		 */
+		next = be32_to_cpu(ri.next) & ROMFH_MASK;
+		if (next && next <= offset) {
+			ret = -EIO;
+			goto error;
+		}
+		offset = next;
 	}
 
 	return d_splice_alias(inode, dentry);
-- 
2.34.1

Re: [PATCH RESEND] romfs: reject directory entry chains that do not move forward

Posted by Jan Kara 2 days, 13 hours ago

Adding Dave Howells since he put himself into the headers of this
filesystem :)

On Fri 18-09-26 17:46:41, Palla Raghunath wrote:
> romfs_lookup() and romfs_readdir() walk a directory's entry chain by
> following ri.next, and stop only once the offset is zero or past the end
> of the image. Nothing stops an entry from pointing at itself, so on a
> crafted image a lookup of a name that is not present spins forever.
> 
> That loop has no cond_resched() and no signal check, so the task cannot
> be killed, and it spins with i_rwsem held and the dentry left in the
> in-lookup state. A second lookup of the same name then waits forever in
> d_alloc_parallel(), and it is that waiter the hung task watchdog
> reports, which makes this look like a dcache problem:
> 
>   INFO: task syz-executor239:5636 blocked for more than 143 seconds.
>    d_alloc_parallel+0xd13/0x16c0 fs/dcache.c:2839
>    __lookup_slow+0x82/0x2f0 fs/namei.c:1904
> 
> Require the chain to move forward and return -EIO if it does not. File
> headers are laid out in increasing order and both loops already treat
> the chain as a bounded forward-only walk, so this only enforces what
> they assume. In romfs_readdir() a cycle never advances ctx->pos and has
> getdents() repeat entries forever, so check there too.
> 
> The hard link chain bounded by commit d30b5a954e0a ("romfs: detect hard
> link cycles") is a separate one. Sangho Lee posted a fix for this chain
> in July 2026 that validates it at inode instantiation; it has had no
> replies.
> 
> Reported-by: syzbot+e8ebfecec06a009dd2d3@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=e8ebfecec06a009dd2d3
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org

I doubt these bugs have any practical relevance so I wouldn't CC stable.
But I guess that's for Christian.

> Signed-off-by: Palla Raghunath <raghunathpalla.0209@gmail.com>

The patch looks good, I'm just not sure we are guaranteed ROMFS directories
are indeed following monotonically increasing disk offset (although that's
the most logical thing to do) - added Dave to CC for that. Otherwise feel
free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/romfs/super.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/romfs/super.c b/fs/romfs/super.c
> index 7f783d6173e8..846b57f265ef 100644
> --- a/fs/romfs/super.c
> +++ b/fs/romfs/super.c
> @@ -143,7 +143,7 @@ static int romfs_readdir(struct file *file, struct dir_context *ctx)
>  {
>  	struct inode *i = file_inode(file);
>  	struct romfs_inode ri;
> -	unsigned long offset, maxoff;
> +	unsigned long offset, maxoff, next;
>  	int j, ino, nextfh;
>  	char fsname[ROMFS_MAXFN];	/* XXX dynamic? */
>  	int ret;
> @@ -191,7 +191,16 @@ static int romfs_readdir(struct file *file, struct dir_context *ctx)
>  			    romfs_dtype_table[nextfh & ROMFH_TYPE]))
>  			goto out;
>  
> -		offset = nextfh & ROMFH_MASK;
> +		/* likewise, a chain that does not move forward would have
> +		 * getdents() repeat entries forever
> +		 */
> +		next = nextfh & ROMFH_MASK;
> +		if (next && next <= offset) {
> +			offset = maxoff;
> +			ctx->pos = offset;
> +			goto out;
> +		}
> +		offset = next;
>  	}
>  out:
>  	return 0;
> @@ -203,7 +212,7 @@ static int romfs_readdir(struct file *file, struct dir_context *ctx)
>  static struct dentry *romfs_lookup(struct inode *dir, struct dentry *dentry,
>  				   unsigned int flags)
>  {
> -	unsigned long offset, maxoff;
> +	unsigned long offset, maxoff, next;
>  	struct inode *inode = NULL;
>  	struct romfs_inode ri;
>  	const char *name;		/* got from dentry */
> @@ -245,8 +254,15 @@ static struct dentry *romfs_lookup(struct inode *dir, struct dentry *dentry,
>  			break;
>  		}
>  
> -		/* next entry */
> -		offset = be32_to_cpu(ri.next) & ROMFH_MASK;
> +		/* next entry; the chain must move forward, or a corrupt image
> +		 * will keep us here forever
> +		 */
> +		next = be32_to_cpu(ri.next) & ROMFH_MASK;
> +		if (next && next <= offset) {
> +			ret = -EIO;
> +			goto error;
> +		}
> +		offset = next;
>  	}
>  
>  	return d_splice_alias(inode, dentry);
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR