[PATCH/RFC] hfs: validate catalog CNIDs before writeback

[PATCH/RFC] hfs: validate catalog CNIDs before writeback

[David Maximiliano Hermitte]

Hi Slava,

Thank you again for the previous review.

I reworked the HFS patch following your feedback. This version
uses a focused CNID validator for catalog records.

The patch now:

- adds hfs_is_valid_cnid();
- validates file and directory CNIDs in hfs_read_inode();
- marks invalid catalog records as bad inodes;
- rejects a bad root inode in hfs_fill_super();
- validates the found catalog record in hfs_cat_find_brec();
- keeps BUG() in hfs_write_inode() for internal impossible state.

Local validation:

- git diff --check: pass
- reverse apply check: pass
- checkpatch strict: 0 errors
- make fs/hfs/: pass
- make bzImage: pass
- QEMU HFS repro: started and completed
- no kernel BUG, Oops, panic, KASAN, UBSAN, or GPF observed

The goal is to treat corrupted on-disk catalog metadata as
filesystem corruption, without crashing the kernel.

Please let me know whether this matches the expected direction.

Thanks,
David

---
diff --git a/fs/hfs/catalog.c b/fs/hfs/catalog.c
index 632c226a3..87aa45db4 100644
--- a/fs/hfs/catalog.c
+++ b/fs/hfs/catalog.c
@@ -184,6 +184,31 @@ int hfs_cat_keycmp(const btree_key *key1, const btree_key *key2)
 
 /* Try to get a catalog entry for given catalog id */
 // move to read_super???
+static int hfs_cat_validate_found_cnid(struct hfs_find_data *fd)
+{
+	hfs_cat_rec rec;
+	int err;
+
+	err = hfs_brec_read(fd, &rec, sizeof(rec));
+	if (err)
+		return err;
+
+	switch (rec.type) {
+	case HFS_CDR_FIL:
+		if (!hfs_is_valid_cnid(be32_to_cpu(rec.file.FlNum), HFS_CDR_FIL))
+			return -EFSCORRUPTED;
+		break;
+	case HFS_CDR_DIR:
+		if (!hfs_is_valid_cnid(be32_to_cpu(rec.dir.DirID), HFS_CDR_DIR))
+			return -EFSCORRUPTED;
+		break;
+	default:
+		return -EFSCORRUPTED;
+	}
+
+	return 0;
+}
+
 int hfs_cat_find_brec(struct super_block *sb, u32 cnid,
 		      struct hfs_find_data *fd)
 {
@@ -208,7 +233,11 @@ int hfs_cat_find_brec(struct super_block *sb, u32 cnid,
 		return -EIO;
 	}
 	memcpy(fd->search_key->cat.CName.name, rec.thread.CName.name, len);
-	return hfs_brec_find(fd);
+	err = hfs_brec_find(fd);
+	if (err)
+		return err;
+
+	return hfs_cat_validate_found_cnid(fd);
 }
 
 
diff --git a/fs/hfs/hfs_fs.h b/fs/hfs/hfs_fs.h
index 49d02524e..d0eb10c4b 100644
--- a/fs/hfs/hfs_fs.h
+++ b/fs/hfs/hfs_fs.h
@@ -186,6 +186,30 @@ extern void hfs_cat_build_key(struct super_block *, btree_key *, u32, const stru
 
 /* dir.c */
 extern const struct file_operations hfs_dir_operations;
+
+/*
+ * Validate the CNID of a catalog record.
+ */
+static inline bool hfs_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:
+	case HFS_BAD_CNID:
+	case HFS_EXCH_CNID:
+		return false;
+	default:
+		return false;
+	}
+}
+
 extern const struct inode_operations hfs_dir_inode_operations;
 
 /* extent.c */
diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index 61ed76d10..90d676dc9 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -344,6 +344,11 @@ static int hfs_read_inode(struct inode *inode, void *data)
 	rec = idata->rec;
 	switch (rec->type) {
 	case HFS_CDR_FIL:
+		if (!hfs_is_valid_cnid(be32_to_cpu(rec->file.FlNum), HFS_CDR_FIL)) {
+			pr_warn_ratelimited("hfs: invalid file CNID %u; run fsck\n",
+					    be32_to_cpu(rec->file.FlNum));
+			goto 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));
@@ -365,6 +370,11 @@ static int hfs_read_inode(struct inode *inode, void *data)
 		inode->i_mapping->a_ops = &hfs_aops;
 		break;
 	case HFS_CDR_DIR:
+		if (!hfs_is_valid_cnid(be32_to_cpu(rec->dir.DirID), HFS_CDR_DIR)) {
+			pr_warn_ratelimited("hfs: invalid directory CNID %u; run fsck\n",
+					    be32_to_cpu(rec->dir.DirID));
+			goto 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;
@@ -375,9 +385,15 @@ static int hfs_read_inode(struct inode *inode, void *data)
 		inode->i_fop = &hfs_dir_operations;
 		break;
 	default:
-		make_bad_inode(inode);
+		pr_warn_ratelimited("hfs: invalid catalog record type %u; run fsck\n",
+				    rec->type);
+		goto bad_inode;
 	}
 	return 0;
+
+bad_inode:
+	make_bad_inode(inode);
+	return 0;
 }
 
 /*
@@ -435,20 +451,20 @@ 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 (!hfs_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;
 	}
 
 	if (HFS_IS_RSRC(inode))
diff --git a/fs/hfs/super.c b/fs/hfs/super.c
index 53800105e..9428cd7fe 100644
--- a/fs/hfs/super.c
+++ b/fs/hfs/super.c
@@ -432,7 +432,7 @@ static int hfs_fill_super(struct super_block *sb, void *data, int silent)
 	res = -EINVAL;
 	root_inode = hfs_iget(sb, &fd.search_key->cat, &rec);
 	hfs_find_exit(&fd);
-	if (!root_inode)
+	if (!root_inode || is_bad_inode(root_inode))
 		goto bail_no_root;
 
 	sb->s_d_op = &hfs_dentry_operations;