[v2] hpfs: reject dirents with an invalid length in hpfs_count_dnodes()

[PATCH v2] hpfs: reject dirents with an invalid length in hpfs_count_dnodes()

Posted by Quchaosheng 1 week, 1 day ago

syzbot reports a slab-use-after-free in hpfs_count_dnodes():

  BUG: KASAN: use-after-free in hpfs_count_dnodes+0x854/0xb20 fs/hpfs/dnode.c:773
  Read of size 2 at addr ffff8880471a64d0 by task syz.0.17/5986

  HPFS: de_next_de: de->length = 0
  HPFS: dnode_end_de: dnode->first_free = 7b3184b6

de_next_de() adds the dirent length to the dirent pointer:

	static inline struct hpfs_dirent *de_next_de (struct hpfs_dirent *de)
	{
	  CHKCOND(...);
	  return (void *) de + le16_to_cpu(de->length);
	}

CHKCOND() only prints, it does not abort the operation.  When a corrupted
dnode contains a dirent whose length is zero, de_next_de() therefore
returns the same pointer it was given, and both loops that walk the dirent
chain in hpfs_count_dnodes() spin on one address until they read past the
end of the dnode.

hpfs_map_dnode() does validate dirent lengths, but only while the "check"
mount option is set, and only up to dnode->first_free, so it does not
cover this walk.  The default "check=normal" mount option documents that
"it should not crash", so the walk must be safe on its own.

Reject a dirent with an invalid length before using it to advance, in both
loops.  The validity check matches the one in hpfs_map_dnode(): at least
0x20, at most 292 and a multiple of four.

This only changes the read path; de_next_de() itself is left alone because
it is also used by the write paths (hpfs_add_to_dnode(), hpfs_add_dirent()
and delete_empty_dnode()), where silently clamping a length would hide
filesystem corruption.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: syzbot+7d1563afac6cb196a444@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=7d1563afac6cb196a444
Signed-off-by: Quchaosheng <quchaosheng000406@163.com>
---
v2: Add the Fixes: tag and use Closes: instead of Link:, the whole bug is
    fixed.

    Resend: v1 never reached the maintainer.  The 163 SMTP relay was
    rejected by artax.karlin.mff.cuni.cz with "554 5.7.1 Helo command
    rejected", and the HPFS entry in MAINTAINERS is stale -- the address
    used for the last fs/hpfs/ commit is mpatocka@redhat.com.  Addressed
    there now.

 fs/hpfs/dnode.c   | 22 ++++++++++++++++++++++
 fs/hpfs/hpfs_fn.h | 14 ++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/fs/hpfs/dnode.c b/fs/hpfs/dnode.c
index 8c6aa060f..224be4124 100644
--- a/fs/hpfs/dnode.c
+++ b/fs/hpfs/dnode.c
@@ -767,6 +767,18 @@ void hpfs_count_dnodes(struct super_block *s, dnode_secno dno, int *n_dnodes,
 				ptr, dno, odno);
 			return;
 		}
+		/*
+		 * de_next_de() returns de itself if the dirent length is
+		 * zero, so a corrupted dnode would make this loop spin on
+		 * one address and read past the end of the dnode.  Require
+		 * a sane length before advancing.
+		 */
+		if (!de_length_valid(de)) {
+			hpfs_brelse4(&qbh);
+			hpfs_error(s, "hpfs_count_dnodes: bad dirent length %u in dnode %08x, dno %08x",
+				(unsigned)le16_to_cpu(de->length), dno, ptr);
+			return;
+		}
 		de = de_next_de(de);
 	}
 	next_de:
@@ -779,6 +791,16 @@ void hpfs_count_dnodes(struct super_block *s, dnode_secno dno, int *n_dnodes,
 	process_de:
 	if (!de->first && !de->last && de->directory && n_subdirs) (*n_subdirs)++;
 	if (!de->first && !de->last && n_items) (*n_items)++;
+	/*
+	 * Same problem here: an invalid dirent length would make the
+	 * "next_de" loop below revisit the same address forever.
+	 */
+	if (!de_length_valid(de)) {
+		hpfs_brelse4(&qbh);
+		hpfs_error(s, "hpfs_count_dnodes: bad dirent length %u in dnode %08x",
+			(unsigned)le16_to_cpu(de->length), dno);
+		return;
+	}
 	if ((de = de_next_de(de)) < dnode_end_de(dnode)) goto next_de;
 	ptr = dno;
 	dno = le32_to_cpu(dnode->up);
diff --git a/fs/hpfs/hpfs_fn.h b/fs/hpfs/hpfs_fn.h
index 237c1c23e..95330fe7a 100644
--- a/fs/hpfs/hpfs_fn.h
+++ b/fs/hpfs/hpfs_fn.h
@@ -126,6 +126,20 @@ static inline struct hpfs_dirent *dnode_end_de (struct dnode *dnode)
 
 /* The dir entry after dir entry de */
 
+/*
+ * A valid dirent length is at least 0x20, at most 292 and a multiple of
+ * four, as checked by hpfs_map_dnode().  de_next_de() adds length to the
+ * dirent pointer, so a length of zero would leave the pointer where it is:
+ * a dirent walk using it would then spin on the same address and read past
+ * the end of the dnode.  A length that is not a multiple of four would
+ * misalign the pointer as well.
+ */
+static inline int de_length_valid (struct hpfs_dirent *de)
+{
+  unsigned int len = le16_to_cpu(de->length);
+  return len >= 0x20 && len <= 292 && !(len & 3);
+}
+
 static inline struct hpfs_dirent *de_next_de (struct hpfs_dirent *de)
 {
   CHKCOND(le16_to_cpu(de->length)>=0x20 && le16_to_cpu(de->length)<0x800,("HPFS: de_next_de: de->length = %x\n",(unsigned)le16_to_cpu(de->length)));
-- 
2.43.0

Re: [PATCH v2] hpfs: reject dirents with an invalid length in hpfs_count_dnodes()

Posted by Mikulas Patocka 1 week, 1 day ago

Hi

This is already checked in hpfs_map_dnode - see the line
if (le16_to_cpu(de->length) > 292 || (le16_to_cpu(de->length) < 32) || (le16_to_cpu(de->length) & 3) || p + le16_to_cpu(de->length) > 2048)

It seems that you disabled checking with the "check=none option" - this 
option does what it's supposed to do - disable filesystem checking.

Mikulas



On Wed, 16 Sep 2026, Quchaosheng wrote:

> syzbot reports a slab-use-after-free in hpfs_count_dnodes():
> 
>   BUG: KASAN: use-after-free in hpfs_count_dnodes+0x854/0xb20 fs/hpfs/dnode.c:773
>   Read of size 2 at addr ffff8880471a64d0 by task syz.0.17/5986
> 
>   HPFS: de_next_de: de->length = 0
>   HPFS: dnode_end_de: dnode->first_free = 7b3184b6
> 
> de_next_de() adds the dirent length to the dirent pointer:
> 
> 	static inline struct hpfs_dirent *de_next_de (struct hpfs_dirent *de)
> 	{
> 	  CHKCOND(...);
> 	  return (void *) de + le16_to_cpu(de->length);
> 	}
> 
> CHKCOND() only prints, it does not abort the operation.  When a corrupted
> dnode contains a dirent whose length is zero, de_next_de() therefore
> returns the same pointer it was given, and both loops that walk the dirent
> chain in hpfs_count_dnodes() spin on one address until they read past the
> end of the dnode.
> 
> hpfs_map_dnode() does validate dirent lengths, but only while the "check"
> mount option is set, and only up to dnode->first_free, so it does not
> cover this walk.  The default "check=normal" mount option documents that
> "it should not crash", so the walk must be safe on its own.
> 
> Reject a dirent with an invalid length before using it to advance, in both
> loops.  The validity check matches the one in hpfs_map_dnode(): at least
> 0x20, at most 292 and a multiple of four.
> 
> This only changes the read path; de_next_de() itself is left alone because
> it is also used by the write paths (hpfs_add_to_dnode(), hpfs_add_dirent()
> and delete_empty_dnode()), where silently clamping a length would hide
> filesystem corruption.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: syzbot+7d1563afac6cb196a444@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=7d1563afac6cb196a444
> Signed-off-by: Quchaosheng <quchaosheng000406@163.com>
> ---
> v2: Add the Fixes: tag and use Closes: instead of Link:, the whole bug is
>     fixed.
> 
>     Resend: v1 never reached the maintainer.  The 163 SMTP relay was
>     rejected by artax.karlin.mff.cuni.cz with "554 5.7.1 Helo command
>     rejected", and the HPFS entry in MAINTAINERS is stale -- the address
>     used for the last fs/hpfs/ commit is mpatocka@redhat.com.  Addressed
>     there now.
> 
>  fs/hpfs/dnode.c   | 22 ++++++++++++++++++++++
>  fs/hpfs/hpfs_fn.h | 14 ++++++++++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/fs/hpfs/dnode.c b/fs/hpfs/dnode.c
> index 8c6aa060f..224be4124 100644
> --- a/fs/hpfs/dnode.c
> +++ b/fs/hpfs/dnode.c
> @@ -767,6 +767,18 @@ void hpfs_count_dnodes(struct super_block *s, dnode_secno dno, int *n_dnodes,
>  				ptr, dno, odno);
>  			return;
>  		}
> +		/*
> +		 * de_next_de() returns de itself if the dirent length is
> +		 * zero, so a corrupted dnode would make this loop spin on
> +		 * one address and read past the end of the dnode.  Require
> +		 * a sane length before advancing.
> +		 */
> +		if (!de_length_valid(de)) {
> +			hpfs_brelse4(&qbh);
> +			hpfs_error(s, "hpfs_count_dnodes: bad dirent length %u in dnode %08x, dno %08x",
> +				(unsigned)le16_to_cpu(de->length), dno, ptr);
> +			return;
> +		}
>  		de = de_next_de(de);
>  	}
>  	next_de:
> @@ -779,6 +791,16 @@ void hpfs_count_dnodes(struct super_block *s, dnode_secno dno, int *n_dnodes,
>  	process_de:
>  	if (!de->first && !de->last && de->directory && n_subdirs) (*n_subdirs)++;
>  	if (!de->first && !de->last && n_items) (*n_items)++;
> +	/*
> +	 * Same problem here: an invalid dirent length would make the
> +	 * "next_de" loop below revisit the same address forever.
> +	 */
> +	if (!de_length_valid(de)) {
> +		hpfs_brelse4(&qbh);
> +		hpfs_error(s, "hpfs_count_dnodes: bad dirent length %u in dnode %08x",
> +			(unsigned)le16_to_cpu(de->length), dno);
> +		return;
> +	}
>  	if ((de = de_next_de(de)) < dnode_end_de(dnode)) goto next_de;
>  	ptr = dno;
>  	dno = le32_to_cpu(dnode->up);
> diff --git a/fs/hpfs/hpfs_fn.h b/fs/hpfs/hpfs_fn.h
> index 237c1c23e..95330fe7a 100644
> --- a/fs/hpfs/hpfs_fn.h
> +++ b/fs/hpfs/hpfs_fn.h
> @@ -126,6 +126,20 @@ static inline struct hpfs_dirent *dnode_end_de (struct dnode *dnode)
>  
>  /* The dir entry after dir entry de */
>  
> +/*
> + * A valid dirent length is at least 0x20, at most 292 and a multiple of
> + * four, as checked by hpfs_map_dnode().  de_next_de() adds length to the
> + * dirent pointer, so a length of zero would leave the pointer where it is:
> + * a dirent walk using it would then spin on the same address and read past
> + * the end of the dnode.  A length that is not a multiple of four would
> + * misalign the pointer as well.
> + */
> +static inline int de_length_valid (struct hpfs_dirent *de)
> +{
> +  unsigned int len = le16_to_cpu(de->length);
> +  return len >= 0x20 && len <= 292 && !(len & 3);
> +}
> +
>  static inline struct hpfs_dirent *de_next_de (struct hpfs_dirent *de)
>  {
>    CHKCOND(le16_to_cpu(de->length)>=0x20 && le16_to_cpu(de->length)<0x800,("HPFS: de_next_de: de->length = %x\n",(unsigned)le16_to_cpu(de->length)));
> -- 
> 2.43.0
>

Re: [PATCH v2] hpfs: reject dirents with an invalid length in hpfs_count_dnodes()

Posted by Quchaosheng 1 week ago

Hi Mikulas,

Thanks for looking at it, and you are right. I checked the syzbot reproducer
and it mounts with

	eas=no,case=asis,check=none,

so this only reproduces with check=none, which is the documented behaviour:
"no fs checks - kernel may crash on corrupted filesystem". The check in
hpfs_map_dnode() does cover the dirent chain that hpfs_count_dnodes() walks
(from offset 20 up to first_free, which is exactly the range the walk covers),
so with the default check=normal the walk is already bounded.

I also should have added an "Assisted-by: LLM ..." tag to this submission per
Documentation/process/coding-assistants.rst, and did not. Sorry for the noise
and for the extra round trip.

Please drop the patch.

Thanks,
Quchaosheng