[PATCH v5] hfs: Validate CNIDs in hfs_read_inode
From: George Anthony Vernon
Date: Sun Mar 29 2026 - 17:00:44 EST
hfs_read_inode previously did not validate CNIDs read from disk, thereby
allowing inodes to be constructed with disallowed CNIDs and placed on
the dirty list, eventually hitting a bug on writeback.
Validate reserved CNIDs as specified for HFS according to
"Inside Macintosh: Files."
This issue was discussed at length on LKML previously, the discussion
is linked below.
This patch was regression tested by issuing various system calls on a
mounted HFS filesystem and validating that file creation, deletion,
reads and writes all work.
Link: https://lore.kernel.org/all/427fcb57-8424-4e52-9f21-7041b2c4ae5b@
I-love.SAKURA.ne.jp/T/
Reported-by: syzbot+97e301b4b82ae803d21b@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
Cc: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
Signed-off-by: George Anthony Vernon <contact@xxxxxxxxxxx>
---
Thanks to Tetsuo and Slava for the reviews on V4. All correct feedback
has been addressed. Tetsuo's patch to validate the superblock on read
remains a very good further improvement.
I have tested this with syzbot's reproducer and my own reproducer.
Previous patch versions were tested with Syzbot also, however I was
unable to test this patch version with Syzbot due to
https://github.com/google/syzkaller/issues/7025.
Changes from V4->V5:
- Free bad inode in hfs_fill_super
- Make sure cnid is initialised before accessing it in warning message
- Use be32_to_cpu helpers for correct accesses of catalog record fields
- Removed syzbot tested-by tag
Changes from V3->V4:
- Remove explicit "do nothing" case statement labels in favor of
implicit default
- Check superblock for bad inode
Changes from V2->V3:
- Use HFS-specific references in preference to TN1150
- Remove Tetsuo's additional superblock check from patch series
- Rename is_valid_cnid() -> is_valid_catalog_record()
- Add static inline hfs_inode_type() helper function
- Do not BUG() on detected bad inode, use pr_warn()
Changes from V1->V2:
- is_valid_cnid prototype now takes u32 and u8 types
- Add fsck advice in dmesg
- Replace make_bad_inode calls in hfs_read_inode with gotos
- Reuse same check in hfs_write_inode
- Disallow HFS_POR_CNID, HFS_BAD_CNID, and HFS_EXCH_CNID
- Add Tetsuo's patch to mine and make it a patch series
fs/hfs/inode.c | 72 +++++++++++++++++++++++++++++++++++++++-----------
fs/hfs/super.c | 4 +++
2 files changed, 60 insertions(+), 16 deletions(-)
diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index 878535db64d6..833261261aba 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -340,6 +340,31 @@ static int hfs_test_inode(struct inode *inode, void *data)
}
}
+/*
+ * is_valid_catalog_record
+ *
+ * Validate the CNID of a catalog record
+ */
+static inline
+bool is_valid_catalog_record(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;
+ default:
+ /* Invalid reserved CNID */
+ break;
+ }
+
+ return false;
+}
+
/*
* hfs_read_inode
*/
@@ -348,6 +373,7 @@ static int hfs_read_inode(struct inode *inode, void *data)
struct hfs_iget_data *idata = data;
struct hfs_sb_info *hsb = HFS_SB(inode->i_sb);
hfs_cat_rec *rec;
+ u32 cnid;
HFS_I(inode)->flags = 0;
HFS_I(inode)->rsrc_inode = NULL;
@@ -369,6 +395,9 @@ static int hfs_read_inode(struct inode *inode, void *data)
rec = idata->rec;
switch (rec->type) {
case HFS_CDR_FIL:
+ cnid = be32_to_cpu(rec->file.FlNum);
+ if (!is_valid_catalog_record(cnid, rec->type))
+ 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));
@@ -377,7 +406,7 @@ static int hfs_read_inode(struct inode *inode, void *data)
rec->file.RPyLen, be16_to_cpu(rec->file.ClpSize));
}
- inode->i_ino = be32_to_cpu(rec->file.FlNum);
+ inode->i_ino = cnid;
inode->i_mode = S_IRUGO | S_IXUGO;
if (!(rec->file.Flags & HFS_FIL_LOCK))
inode->i_mode |= S_IWUGO;
@@ -390,7 +419,10 @@ static int hfs_read_inode(struct inode *inode, void *data)
inode->i_mapping->a_ops = &hfs_aops;
break;
case HFS_CDR_DIR:
- inode->i_ino = be32_to_cpu(rec->dir.DirID);
+ cnid = be32_to_cpu(rec->dir.DirID);
+ if (!is_valid_catalog_record(cnid, rec->type))
+ goto make_bad_inode;
+ inode->i_ino = cnid;
inode->i_size = be16_to_cpu(rec->dir.Val) + 2;
HFS_I(inode)->fs_blocks = 0;
inode->i_mode = S_IFDIR | (S_IRWXUGO & ~hsb->s_dir_umask);
@@ -399,8 +431,13 @@ 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("Invalid inode with cnid %lu\n", cnid);
+ pr_warn("Volume is probably corrupted, try performing fsck.\n");
+ fallthrough;
default:
make_bad_inode(inode);
+ break;
}
return 0;
}
@@ -448,6 +485,11 @@ void hfs_inode_write_fork(struct inode *inode, struct hfs_extent *ext,
HFS_SB(inode->i_sb)->alloc_blksz);
}
+static inline u8 hfs_inode_type(struct inode *inode)
+{
+ return S_ISDIR(inode->i_mode) ? HFS_CDR_DIR : HFS_CDR_FIL;
+}
+
int hfs_write_inode(struct inode *inode, struct writeback_control *wbc)
{
struct inode *main_inode = inode;
@@ -460,20 +502,18 @@ 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_catalog_record(inode->i_ino, hfs_inode_type(inode)))
+ return -EIO;
+
+ 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 a4f2a2bfa6d3..18d49db551ac 100644
--- a/fs/hfs/super.c
+++ b/fs/hfs/super.c
@@ -371,6 +371,10 @@ static int hfs_fill_super(struct super_block *sb, struct fs_context *fc)
hfs_find_exit(&fd);
if (!root_inode)
goto bail_no_root;
+ if (is_bad_inode(root_inode)) {
+ iput(root_inode);
+ goto bail_no_root;
+ }
set_default_d_op(sb, &hfs_dentry_operations);
res = -ENOMEM;
--
2.53.0