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;