Re: [PATCH v2] hfs: validate corrupt catalog and btree metadata before hfs_write_inode()

Re: [PATCH v2] hfs: validate corrupt catalog and btree metadata before hfs_write_inode()

[David Maximiliano Hermitte]

Hello Slava, Jori, all,

I am following up with a refined test patch for the same HFS syzbot reproducer.

This variant does not change hfs_write_inode(), hfs_test_inode(), or the BUG() site. It rejects corrupted catalog/B-tree metadata earlier:

- hfs_brec_read(): rejects invalid entry offset/length, caches node_size locally, and uses overflow-safe bounds arithmetic.
- hfs_read_inode(): rejects invalid file/directory CNIDs before writeback.

Local validation on commit 840ef6c78e6a:

- git diff --check: pass
- checkpatch.pl --strict: 0 errors, 0 warnings
- make -j2 fs/hfs/: pass
- bzImage built
- QEMU/TCG with the same public syzbot ReproC:
  - BEFORE: repro_started=true, kernel_bug_seen=true
  - AFTER: repro_started=true, repro_done=true, kernel_bug_seen=false, hfs_write_inode_seen=false, kasan_seen=false, oops_seen=false
  - verdict=REPRO_RAN_NO_BUG_SEEN

I have prepared this refined variant because the hfs_brec_read() bounds check now validates entryoffset before computing node_size - entryoffset, caches node_size locally, and rejects entryoffset >= node_size explicitly. I am including it inline because it avoids potential integer overflow in the bounds check while preserving the same QEMU validation result.

I am sending it as a test patch / direction check, not as a final maintainer-ready fix. The point is that validating corrupt catalog/B-tree metadata before hfs_write_inode() prevents this reproducer from reaching the BUG() endpoint.

I would appreciate your opinion on whether this placement/direction is closer to the expected HFS fix.

Best regards,
David

Patch follows:

diff --git a/fs/hfs/bfind.c b/fs/hfs/bfind.c
index d56e47bdc517..e48a7110e342 100644
--- a/fs/hfs/bfind.c
+++ b/fs/hfs/bfind.c
@@ -170,12 +170,20 @@ int hfs_brec_find(struct hfs_find_data *fd)
 int hfs_brec_read(struct hfs_find_data *fd, void *rec, u32 rec_len)
 {
 	int res;
+	u32 node_size;
 
 	res = hfs_brec_find(fd);
 	if (res)
 		return res;
+	node_size = fd->bnode->tree->node_size;
+	if (fd->entryoffset < 0 || fd->entrylength <= 0)
+		return -EFSCORRUPTED;
+	if (fd->entryoffset >= node_size)
+		return -EFSCORRUPTED;
+	if (fd->entrylength > node_size - fd->entryoffset)
+		return -EFSCORRUPTED;
 	if (fd->entrylength > rec_len)
-		return -EINVAL;
+		return -EFSCORRUPTED;
 	hfs_bnode_read(fd->bnode, rec, fd->entryoffset, fd->entrylength);
 	return 0;
 }
diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index ac4a9055c5c0..7283b9541064 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -367,6 +367,9 @@ static int hfs_read_inode(struct inode *inode, void *data)
 	rec = idata->rec;
 	switch (rec->type) {
 	case HFS_CDR_FIL:
+		/* Reject corrupted user file CNIDs before writeback. */
+		if (be32_to_cpu(rec->file.FlNum) < HFS_FIRSTUSER_CNID)
+			return -EFSCORRUPTED;
 		if (!HFS_IS_RSRC(inode)) {
 			hfs_inode_read_fork(inode, rec->file.ExtRec, rec->file.LgLen,
 					    rec->file.PyLen, be16_to_cpu(rec->file.ClpSize));
@@ -390,6 +393,10 @@ static int hfs_read_inode(struct inode *inode, void *data)
 		inode->i_mapping->a_ops = &hfs_aops;
 		break;
 	case HFS_CDR_DIR:
+		/* Only the root directory may use a reserved CNID here. */
+		if (be32_to_cpu(rec->dir.DirID) < HFS_FIRSTUSER_CNID &&
+		    be32_to_cpu(rec->dir.DirID) != HFS_ROOT_CNID)
+			return -EFSCORRUPTED;
 		inode->i_ino = be32_to_cpu(rec->dir.DirID);
 		inode->i_size = be16_to_cpu(rec->dir.Val) + 2;
 		HFS_I(inode)->fs_blocks = 0;

Re: [PATCH v2] hfs: validate corrupt catalog and btree metadata before hfs_write_inode()

[Viacheslav Dubeyko]

On Wed, 2026-06-24 at 13:54 +0000, David Maximiliano Hermitte wrote:
> Hello Slava, Jori, all,
> 
> I am following up with a refined test patch for the same HFS syzbot
> reproducer.
> 
> This variant does not change hfs_write_inode(), hfs_test_inode(), or
> the BUG() site. It rejects corrupted catalog/B-tree metadata earlier:
> 
> - hfs_brec_read(): rejects invalid entry offset/length, caches
> node_size locally, and uses overflow-safe bounds arithmetic.
> - hfs_read_inode(): rejects invalid file/directory CNIDs before
> writeback.
> 
> Local validation on commit 840ef6c78e6a:
> 
> - git diff --check: pass
> - checkpatch.pl --strict: 0 errors, 0 warnings
> - make -j2 fs/hfs/: pass
> - bzImage built
> - QEMU/TCG with the same public syzbot ReproC:
>   - BEFORE: repro_started=true, kernel_bug_seen=true
>   - AFTER: repro_started=true, repro_done=true,
> kernel_bug_seen=false, hfs_write_inode_seen=false, kasan_seen=false,
> oops_seen=false
>   - verdict=REPRO_RAN_NO_BUG_SEEN
> 
> I have prepared this refined variant because the hfs_brec_read()
> bounds check now validates entryoffset before computing node_size -
> entryoffset, caches node_size locally, and rejects entryoffset >=
> node_size explicitly. I am including it inline because it avoids
> potential integer overflow in the bounds check while preserving the
> same QEMU validation result.
> 
> I am sending it as a test patch / direction check, not as a final
> maintainer-ready fix. The point is that validating corrupt catalog/B-
> tree metadata before hfs_write_inode() prevents this reproducer from
> reaching the BUG() endpoint.
> 
> I would appreciate your opinion on whether this placement/direction
> is closer to the expected HFS fix.
> 
> Best regards,
> David
> 
> Patch follows:
> 
> diff --git a/fs/hfs/bfind.c b/fs/hfs/bfind.c
> index d56e47bdc517..e48a7110e342 100644
> --- a/fs/hfs/bfind.c
> +++ b/fs/hfs/bfind.c
> @@ -170,12 +170,20 @@ int hfs_brec_find(struct hfs_find_data *fd)
>  int hfs_brec_read(struct hfs_find_data *fd, void *rec, u32 rec_len)
>  {
>  	int res;
> +	u32 node_size;
>  
>  	res = hfs_brec_find(fd);
>  	if (res)
>  		return res;
> +	node_size = fd->bnode->tree->node_size;

These multiple dereferences make me really nervous. As some pointer
potentially could be NULL as node_size could be not valid.

> +	if (fd->entryoffset < 0 || fd->entrylength <= 0)
> +		return -EFSCORRUPTED;
> +	if (fd->entryoffset >= node_size)
> +		return -EFSCORRUPTED;
> +	if (fd->entrylength > node_size - fd->entryoffset)
> +		return -EFSCORRUPTED;
>  	if (fd->entrylength > rec_len)
> -		return -EINVAL;
> +		return -EFSCORRUPTED;

These values are not records from the volume. So, these values could be
wrong because of file system logic bugs but because the volume is
corrupted. And, frankly speaking, I don't see connection between the
issue that you are trying to solve and these checks.

>  	hfs_bnode_read(fd->bnode, rec, fd->entryoffset, fd-
> >entrylength);
>  	return 0;
>  }
> diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
> index ac4a9055c5c0..7283b9541064 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -367,6 +367,9 @@ static int hfs_read_inode(struct inode *inode,
> void *data)
>  	rec = idata->rec;
>  	switch (rec->type) {
>  	case HFS_CDR_FIL:
> +		/* Reject corrupted user file CNIDs before
> writeback. */
> +		if (be32_to_cpu(rec->file.FlNum) <
> HFS_FIRSTUSER_CNID)
> +			return -EFSCORRUPTED;

Please, see my explanation below.

>  		if (!HFS_IS_RSRC(inode)) {
>  			hfs_inode_read_fork(inode, rec->file.ExtRec,
> rec->file.LgLen,
>  					    rec->file.PyLen,
> be16_to_cpu(rec->file.ClpSize));
> @@ -390,6 +393,10 @@ static int hfs_read_inode(struct inode *inode,
> void *data)
>  		inode->i_mapping->a_ops = &hfs_aops;
>  		break;
>  	case HFS_CDR_DIR:
> +		/* Only the root directory may use a reserved CNID
> here. */
> +		if (be32_to_cpu(rec->dir.DirID) < HFS_FIRSTUSER_CNID
> &&
> +		    be32_to_cpu(rec->dir.DirID) != HFS_ROOT_CNID)
> +			return -EFSCORRUPTED;

Please, see my explanation below.

>  		inode->i_ino = be32_to_cpu(rec->dir.DirID);
>  		inode->i_size = be16_to_cpu(rec->dir.Val) + 2;
>  		HFS_I(inode)->fs_blocks = 0;

If we have corrupted Catalog File, then, the root folder record could
be corrupted too. So, first of all, we need to manage this case in
hfs_fill_super() [1]. If something is going wrong, then hfs_iget() can
return bad inode [2,3] or NULL [4]. So, hfs_fill_super() shoudl be
ready to manage this situation and it should not mount volume with
corrupted root folder.

Also, we need to check the CNID validity:

+/*
+ * is_valid_cnid
+ *
+ * Validate the CNID of a catalog record
+ */
+static inline
+bool is_valid_cnid(u32 cnid, u8 type)
+{
+	if (likely(cnid >= HFS_FIRSTUSER_CNID))
+		return true;
+
+	switch (cnid) {
+	case HFS_ROOT_CNID:
+		return type == HFS_CDR_DIR;
+	case HFS_EXT_CNID:
+	case HFS_CAT_CNID:
+		return type == HFS_CDR_FIL;
+	case HFS_POR_CNID:
+		/* No valid record with this CNID */
+		break;
+	case HFS_BAD_CNID:
+	case HFS_EXCH_CNID:
+		/* Not implemented */
+		break;
+	default:
+		/* Invalid reserved CNID */
+		break;
+	}
+
+	return false;
+}

And hfs_read_inode() should execute this check:

 /*
  * hfs_read_inode
  */
@@ -350,6 +382,8 @@ static int hfs_read_inode(struct inode *inode, void
*data)
 	rec = idata->rec;
 	switch (rec->type) {
 	case HFS_CDR_FIL:
+		if (!is_valid_cnid(rec->file.FlNum, HFS_CDR_FIL))
+			goto make_bad_inode;
 		if (!HFS_IS_RSRC(inode)) {
 			hfs_inode_read_fork(inode, rec->file.ExtRec,
rec->file.LgLen,
 					    rec->file.PyLen,
be16_to_cpu(rec->file.ClpSize));
@@ -371,6 +405,8 @@ static int hfs_read_inode(struct inode *inode, void
*data)
 		inode->i_mapping->a_ops = &hfs_aops;
 		break;
 	case HFS_CDR_DIR:
+		if (!is_valid_cnid(rec->dir.DirID, HFS_CDR_DIR))
+			goto make_bad_inode;
 		inode->i_ino = be32_to_cpu(rec->dir.DirID);
 		inode->i_size = be16_to_cpu(rec->dir.Val) + 2;
 		HFS_I(inode)->fs_blocks = 0;
@@ -380,8 +416,12 @@ static int hfs_read_inode(struct inode *inode,
void *data)
 		inode->i_op = &hfs_dir_inode_operations;
 		inode->i_fop = &hfs_dir_operations;
 		break;
+	make_bad_inode:
+		pr_warn("rejected cnid %lu. Volume is probably
corrupted, try performing fsck.\n", inode->i_ino);
+		fallthrough;
 	default:
 		make_bad_inode(inode);
+		break;
 	}
 	return 0;
 }

Potentially, hfs_write_inode() could employ this check too:

@@ -441,20 +481,19 @@ int hfs_write_inode(struct inode *inode, struct
writeback_control *wbc)
 	if (res)
 		return res;
 
-	if (inode->i_ino < HFS_FIRSTUSER_CNID) {
-		switch (inode->i_ino) {
-		case HFS_ROOT_CNID:
-			break;
-		case HFS_EXT_CNID:
-			hfs_btree_write(HFS_SB(inode->i_sb)-
>ext_tree);
-			return 0;
-		case HFS_CAT_CNID:
-			hfs_btree_write(HFS_SB(inode->i_sb)-
>cat_tree);
-			return 0;
-		default:
-			BUG();
-			return -EIO;
-		}
+	if (!is_valid_cnid(inode->i_ino,
+			   S_ISDIR(inode->i_mode) ? HFS_CDR_DIR :
HFS_CDR_FIL))
+		BUG();
+
+	switch (inode->i_ino) {
+	case HFS_EXT_CNID:
+		hfs_btree_write(HFS_SB(inode->i_sb)->ext_tree);
+		return 0;
+	case HFS_CAT_CNID:
+		hfs_btree_write(HFS_SB(inode->i_sb)->cat_tree);
+		return 0;
+	default:
+		break;
 	}

The most tricky part is to check the CNID in file or folder record.
File and folder records always have a key that contains a non-empty
nodeName. The key for a thread record is the file's or folder's CNID
(not the CNID of the parent folder) and an empty (zero length)
nodeName. This allows a file or folder to by found using just the CNID.
The thread record contains the parentID and nodeName field of the file
or folder itself. Finding a file or folder by its CNID is a two-step
process. The first step is to use the CNID to look up the thread record
for the file or folder. This yields the file or folder's parent folder
ID and name. The second step is to use that information to look up the
real file or folder record.

So, the key of thread record could contain the valid CNID but the found
file or folder record could contain corrupted CNID value. So, my vision
is that hfs_cat_find_brec() needs to check that found record contains
valid CNID value.

Thanks,
Slava. 

[1] https://elixir.bootlin.com/linux/v7.1/source/fs/hfs/super.c#L370
[2] https://elixir.bootlin.com/linux/v7.1/source/fs/hfs/inode.c#L433
[3] https://elixir.bootlin.com/linux/v7.1/source/fs/hfs/inode.c#L403
[4] https://elixir.bootlin.com/linux/v7.1/source/fs/hfs/inode.c#L431