Re: [PATCH v4] hfs: validate catalog CNIDs before instantiating inodes

From: Viacheslav Dubeyko

Date: Wed Jul 08 2026 - 15:45:26 EST

On Tue, 2026-07-07 at 23:28 +0000, David Maximiliano Hermitte wrote:
> hfs_cat_find_brec() first resolves a catalog thread record by CNID
> and
> then looks up the corresponding catalog record by parent/name. On a
> corrupted filesystem image, the second lookup may find a record whose
> CNID does not match the CNID that was requested.
>
> Validate the record found by the second lookup before returning it.
> Read the already-found record with hfs_bnode_read(), require the
> exact
> fixed size for file and directory records, reject other record types,
> and verify that the stored CNID matches the requested CNID.
>
> Also validate reserved CNIDs in hfs_read_inode() before populating
> the
> inode, propagate hfs_read_inode() failures from the resource-fork
> lookup
> path, and reject bad resource-fork and root inodes.
>
> Keep hfs_write_inode() unchanged, so corrupted catalog records are
> rejected before reaching its existing reserved-CNID BUG() path.
>
> Reported-by: syzbot+97e301b4b82ae803d21b@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
> Cc: George Anthony Vernon <contact@xxxxxxxxxxx>
> Cc: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: David Maximiliano Hermitte <davemadmaxxx@xxxxxxxxx>
> ---
> Changes in v4:
> - Combine the catalog record length validation into one check.
> - Require exact sizes for file and directory catalog records.
> - Keep hfs_write_inode() unchanged following maintainer feedback.
> - Revalidate the exact final patch with the HFS build and QEMU repro.
>
> Changes in v3:
> - Drop the stray blank line in hfs_file_lookup().
>
> Changes in v2:
> - Use size_t for the catalog record length.
> - Reject catalog records larger than hfs_cat_rec.
> - Treat non-file/non-directory records as invalid.
> - Propagate hfs_read_inode() failures from resource-fork lookup.
> - Reject bad resource-fork and root inodes.
>
> fs/hfs/catalog.c | 41 ++++++++++++++++++++++++++++++++++++++++-
> fs/hfs/hfs_fs.h | 19 +++++++++++++++++++
> fs/hfs/inode.c   | 12 +++++++++---
> fs/hfs/super.c   | 5 +++++
> 4 files changed, 73 insertions(+), 4 deletions(-)
>
> diff --git a/fs/hfs/catalog.c b/fs/hfs/catalog.c
> index 1bfa36d71e24..e672a4ba6dc7 100644
> --- a/fs/hfs/catalog.c
> +++ b/fs/hfs/catalog.c
> @@ -182,6 +182,40 @@ int hfs_cat_keycmp(const btree_key *key1, const
> btree_key *key2)
> key2->cat.CName.name, key2-
> >cat.CName.len);
> }
>
> +static int hfs_cat_validate_found_cnid(struct hfs_find_data *fd, u32
> cnid)
> +{
> + hfs_cat_rec rec;
> + u32 found_cnid;
> + size_t rec_len;
> +
> + rec_len = fd->entrylength;
> + if (rec_len <= 0 || rec_len > sizeof(rec))

If we still have size_t rec_len, then we cannot keep negative values
because fd->entrylength has int data type. So, this rec_len <= 0 is not
correct now. We need to rework it somehow. Maybe, ssize_t here?

> + return -EIO;
> +
> + memset(&rec, 0, sizeof(rec));
> + hfs_bnode_read(fd->bnode, &rec, fd->entryoffset, rec_len);
> +
> + switch (rec.type) {
> + case HFS_CDR_FIL:
> + if (rec_len != sizeof(struct hfs_cat_file))
> + return -EIO;
> + found_cnid = be32_to_cpu(rec.file.FlNum);
> + break;
> + case HFS_CDR_DIR:
> + if (rec_len != sizeof(struct hfs_cat_dir))
> + return -EIO;
> + found_cnid = be32_to_cpu(rec.dir.DirID);
> + break;
> + default:
> + return -EIO;
> + }
> +
> + if (found_cnid != cnid)
> + return -EIO;
> +
> + return 0;
> +}
> +
> /* Try to get a catalog entry for given catalog id */
> // move to read_super???
> int hfs_cat_find_brec(struct super_block *sb, u32 cnid,
> @@ -208,7 +242,12 @@ 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);
> +
> + res = hfs_brec_find(fd);
> + if (res)
> + return res;
> +
> + return hfs_cat_validate_found_cnid(fd, cnid);
> }
>
> static inline
> diff --git a/fs/hfs/hfs_fs.h b/fs/hfs/hfs_fs.h
> index f3624514fcb0..670638f17438 100644
> --- a/fs/hfs/hfs_fs.h
> +++ b/fs/hfs/hfs_fs.h
> @@ -155,6 +155,25 @@ extern int hfs_cat_move(u32 cnid, struct inode
> *src_dir,
> extern void hfs_cat_build_key(struct super_block *sb, btree_key
> *key,
>       u32 parent, const struct qstr *name);
>
> +/*
> + * 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;
> + default:
> + return false;
> + }
> +}
> +
> /* dir.c */
> extern const struct file_operations hfs_dir_operations;
> extern const struct inode_operations hfs_dir_inode_operations;
> diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
> index ac4a9055c5c0..ee10a9257a13 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:
> + if (!hfs_is_valid_cnid(be32_to_cpu(rec->file.FlNum),
> rec->type))
> + return -EIO;
> +
> 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,9 @@ 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),
> rec->type))
> + return -EIO;
> +
> 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;
> @@ -571,12 +577,12 @@ static struct dentry *hfs_file_lookup(struct
> inode *dir, struct dentry *dentry,
> res = hfs_brec_read(&fd, &rec, sizeof(rec));
> if (!res) {
> struct hfs_iget_data idata = { NULL, &rec };
> - hfs_read_inode(inode, &idata);
> + res = hfs_read_inode(inode, &idata);
> }
> hfs_find_exit(&fd);
> - if (res) {
> + if (res || is_bad_inode(inode)) {
> iput(inode);
> - return ERR_PTR(res);
> + return ERR_PTR(-EIO);

I don't think that it's correct modification. We need to return res if
we have the error code there. And we could return -EIO if we have bad
inode. Do you agree that we need to rework it?

Thanks,
Slava.

> }
> HFS_I(inode)->rsrc_inode = dir;
> HFS_I(dir)->rsrc_inode = inode;
> diff --git a/fs/hfs/super.c b/fs/hfs/super.c
> index a466c401f6bb..98b80795bcde 100644
> --- a/fs/hfs/super.c
> +++ b/fs/hfs/super.c
> @@ -372,6 +372,11 @@ static int hfs_fill_super(struct super_block
> *sb, struct fs_context *fc)
> 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;
> sb->s_root = d_make_root(root_inode);