* [PATCH] hfs: validate catalog CNIDs before instantiating inodes
@ 2026-07-02 18:38 David Maximiliano Hermitte
2026-07-02 23:28 ` Tetsuo Handa
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: David Maximiliano Hermitte @ 2026-07-02 18:38 UTC (permalink / raw)
To: Viacheslav Dubeyko, Jori Koolstra
Cc: George Anthony Vernon, Tetsuo Handa, John Paul Adrian Glaubitz,
Yangtao Li, linux-fsdevel, linux-kernel,
syzbot+97e301b4b82ae803d21b, David Maximiliano Hermitte
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 catalog record found by the second lookup before returning
it to callers. Inspect the already-found record with hfs_bnode_read(),
not hfs_brec_read(), and reject records whose CNID is invalid for their
record type or does not match the requested CNID.
Also validate CNIDs in hfs_read_inode() before the inode is populated,
and propagate hfs_read_inode() errors from the resource-fork lookup path.
For the root inode path, require the root catalog record to be a directory
with DirID == HFS_ROOT_CNID, and drop the root inode reference if it was
instantiated as a bad inode.
This keeps hfs_write_inode() unchanged and prevents corrupted catalog
records from reaching the existing reserved-CNID BUG() path during
writeback.
Reported-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
Cc: George Anthony Vernon <contact@gvernon.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: David Maximiliano Hermitte <davemadmaxxx@gmail.com>
---
fs/hfs/catalog.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
fs/hfs/hfs_fs.h | 19 +++++++++++++++++++
fs/hfs/inode.c | 9 ++++++++-
fs/hfs/super.c | 8 +++++++-
4 files changed, 81 insertions(+), 3 deletions(-)
diff --git a/fs/hfs/catalog.c b/fs/hfs/catalog.c
index 1bfa36d71e24..fa2a0a5975e3 100644
--- a/fs/hfs/catalog.c
+++ b/fs/hfs/catalog.c
@@ -182,6 +182,47 @@ 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;
+ unsigned int rec_len;
+ size_t cnid_off;
+
+ if (fd->entrylength <= 0)
+ return -EIO;
+
+ if ((unsigned int)fd->entrylength > sizeof(rec))
+ rec_len = sizeof(rec);
+ else
+ rec_len = fd->entrylength;
+
+ memset(&rec, 0, sizeof(rec));
+ hfs_bnode_read(fd->bnode, &rec, fd->entryoffset, rec_len);
+
+ switch (rec.type) {
+ case HFS_CDR_FIL:
+ cnid_off = offsetof(struct hfs_cat_file, FlNum);
+ if ((size_t)rec_len < cnid_off + sizeof(rec.file.FlNum))
+ return -EIO;
+ found_cnid = be32_to_cpu(rec.file.FlNum);
+ break;
+ case HFS_CDR_DIR:
+ cnid_off = offsetof(struct hfs_cat_dir, DirID);
+ if ((size_t)rec_len < cnid_off + sizeof(rec.dir.DirID))
+ return -EIO;
+ found_cnid = be32_to_cpu(rec.dir.DirID);
+ break;
+ default:
+ return -EIO;
+ }
+
+ if (!hfs_is_valid_cnid(found_cnid, rec.type) || 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 +249,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..c685f1bb7009 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,7 +577,8 @@ 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) {
diff --git a/fs/hfs/super.c b/fs/hfs/super.c
index a466c401f6bb..5275936304c7 100644
--- a/fs/hfs/super.c
+++ b/fs/hfs/super.c
@@ -361,7 +361,8 @@ static int hfs_fill_super(struct super_block *sb, struct fs_context *fc)
goto bail_hfs_find;
}
hfs_bnode_read(fd.bnode, &rec, fd.entryoffset, fd.entrylength);
- if (rec.type != HFS_CDR_DIR)
+ if (rec.type != HFS_CDR_DIR ||
+ be32_to_cpu(rec.dir.DirID) != HFS_ROOT_CNID)
res = -EIO;
}
if (res)
@@ -372,6 +373,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);
--
2.43.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] hfs: validate catalog CNIDs before instantiating inodes
2026-07-02 18:38 [PATCH] hfs: validate catalog CNIDs before instantiating inodes David Maximiliano Hermitte
@ 2026-07-02 23:28 ` Tetsuo Handa
2026-07-02 23:59 ` Viacheslav Dubeyko
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: Tetsuo Handa @ 2026-07-02 23:28 UTC (permalink / raw)
To: David Maximiliano Hermitte, Viacheslav Dubeyko, Jori Koolstra
Cc: George Anthony Vernon, John Paul Adrian Glaubitz, Yangtao Li,
linux-fsdevel, linux-kernel, syzbot+97e301b4b82ae803d21b
Sashiko has some comments on this patch:
https://sashiko.dev/#/patchset/20260702183800.1119202-1-davemadmaxxx%40gmail.com
On 2026/07/03 3:38, 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 catalog record found by the second lookup before returning
> it to callers. Inspect the already-found record with hfs_bnode_read(),
> not hfs_brec_read(), and reject records whose CNID is invalid for their
> record type or does not match the requested CNID.
>
> Also validate CNIDs in hfs_read_inode() before the inode is populated,
> and propagate hfs_read_inode() errors from the resource-fork lookup path.
> For the root inode path, require the root catalog record to be a directory
> with DirID == HFS_ROOT_CNID, and drop the root inode reference if it was
> instantiated as a bad inode.
>
> This keeps hfs_write_inode() unchanged and prevents corrupted catalog
> records from reaching the existing reserved-CNID BUG() path during
> writeback.
>
> Reported-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
> Cc: George Anthony Vernon <contact@gvernon.com>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: David Maximiliano Hermitte <davemadmaxxx@gmail.com>
> ---
> fs/hfs/catalog.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
> fs/hfs/hfs_fs.h | 19 +++++++++++++++++++
> fs/hfs/inode.c | 9 ++++++++-
> fs/hfs/super.c | 8 +++++++-
> 4 files changed, 81 insertions(+), 3 deletions(-)
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] hfs: validate catalog CNIDs before instantiating inodes
2026-07-02 18:38 [PATCH] hfs: validate catalog CNIDs before instantiating inodes David Maximiliano Hermitte
2026-07-02 23:28 ` Tetsuo Handa
@ 2026-07-02 23:59 ` Viacheslav Dubeyko
2026-07-06 9:59 ` [PATCH v3 RESEND] " David Maximiliano Hermitte
2026-07-07 23:28 ` [PATCH v4] " David Maximiliano Hermitte
3 siblings, 0 replies; 11+ messages in thread
From: Viacheslav Dubeyko @ 2026-07-02 23:59 UTC (permalink / raw)
To: David Maximiliano Hermitte, Jori Koolstra
Cc: George Anthony Vernon, Tetsuo Handa, John Paul Adrian Glaubitz,
Yangtao Li, linux-fsdevel, linux-kernel,
syzbot+97e301b4b82ae803d21b
On Thu, 2026-07-02 at 18:38 +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 catalog record found by the second lookup before
> returning
> it to callers. Inspect the already-found record with
> hfs_bnode_read(),
> not hfs_brec_read(), and reject records whose CNID is invalid for
> their
> record type or does not match the requested CNID.
>
> Also validate CNIDs in hfs_read_inode() before the inode is
> populated,
> and propagate hfs_read_inode() errors from the resource-fork lookup
> path.
> For the root inode path, require the root catalog record to be a
> directory
> with DirID == HFS_ROOT_CNID, and drop the root inode reference if it
> was
> instantiated as a bad inode.
>
> This keeps hfs_write_inode() unchanged and prevents corrupted catalog
> records from reaching the existing reserved-CNID BUG() path during
> writeback.
>
> Reported-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
> Cc: George Anthony Vernon <contact@gvernon.com>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: David Maximiliano Hermitte <davemadmaxxx@gmail.com>
> ---
> fs/hfs/catalog.c | 48
> +++++++++++++++++++++++++++++++++++++++++++++++-
> fs/hfs/hfs_fs.h | 19 +++++++++++++++++++
> fs/hfs/inode.c | 9 ++++++++-
> fs/hfs/super.c | 8 +++++++-
> 4 files changed, 81 insertions(+), 3 deletions(-)
>
> diff --git a/fs/hfs/catalog.c b/fs/hfs/catalog.c
> index 1bfa36d71e24..fa2a0a5975e3 100644
> --- a/fs/hfs/catalog.c
> +++ b/fs/hfs/catalog.c
> @@ -182,6 +182,47 @@ 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;
> + unsigned int rec_len;
I prefer to see the size_t here.
> + size_t cnid_off;
> +
> + if (fd->entrylength <= 0)
> + return -EIO;
> +
> + if ((unsigned int)fd->entrylength > sizeof(rec))
The hfs_cat_rec union is big enough to keep any type of record in
Catalog File. If fd->entrylength is bigger one, then it sounds like a
incorrect behavior or corruption. We cannot silently process such
situation.
> + rec_len = sizeof(rec);
> + else
> + rec_len = fd->entrylength;
> +
> + memset(&rec, 0, sizeof(rec));
> + hfs_bnode_read(fd->bnode, &rec, fd->entryoffset, rec_len);
> +
> + switch (rec.type) {
> + case HFS_CDR_FIL:
> + cnid_off = offsetof(struct hfs_cat_file, FlNum);
> + if ((size_t)rec_len < cnid_off +
> sizeof(rec.file.FlNum))
> + return -EIO;
If you would like to check that size of the record, then you don't need
to do this weird logic.
typedef union hfs_cat_rec {
s8 type; /* The type of entry */
struct hfs_cat_file file;
struct hfs_cat_dir dir;
struct hfs_cat_thread thread;
} hfs_cat_rec;
You can simply compare with particular structure size.
> + found_cnid = be32_to_cpu(rec.file.FlNum);
> + break;
> + case HFS_CDR_DIR:
> + cnid_off = offsetof(struct hfs_cat_dir, DirID);
> + if ((size_t)rec_len < cnid_off +
> sizeof(rec.dir.DirID))
> + return -EIO;
Ditto.
> + found_cnid = be32_to_cpu(rec.dir.DirID);
> + break;
> + default:
> + return -EIO;
Are we sure that thread record cannot be checked by this logic?
> + }
> +
> + if (!hfs_is_valid_cnid(found_cnid, rec.type) || found_cnid
> != cnid)
We don't need to call hfs_is_valid_cnid() because found_cnid != cnid is
completely enough.
> + 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 +249,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..c685f1bb7009 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,7 +577,8 @@ 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);
> +
Do we really need empty line here?
> + res = hfs_read_inode(inode, &idata);
> }
> hfs_find_exit(&fd);
I believe we need something like this here:
if (res || is_bad_inode(inode)) {
iput(inode);
return ERR_PTR(-EIO);
}
> if (res) {
> diff --git a/fs/hfs/super.c b/fs/hfs/super.c
> index a466c401f6bb..5275936304c7 100644
> --- a/fs/hfs/super.c
> +++ b/fs/hfs/super.c
> @@ -361,7 +361,8 @@ static int hfs_fill_super(struct super_block *sb,
> struct fs_context *fc)
> goto bail_hfs_find;
> }
> hfs_bnode_read(fd.bnode, &rec, fd.entryoffset,
> fd.entrylength);
> - if (rec.type != HFS_CDR_DIR)
> + if (rec.type != HFS_CDR_DIR ||
> + be32_to_cpu(rec.dir.DirID) != HFS_ROOT_CNID)
This check is completely not necessary here because
hfs_cat_validate_found_cnid() will execute the CNID check.
Thanks,
Slava.
> res = -EIO;
> }
> if (res)
> @@ -372,6 +373,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);
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 RESEND] hfs: validate catalog CNIDs before instantiating inodes
2026-07-02 18:38 [PATCH] hfs: validate catalog CNIDs before instantiating inodes David Maximiliano Hermitte
2026-07-02 23:28 ` Tetsuo Handa
2026-07-02 23:59 ` Viacheslav Dubeyko
@ 2026-07-06 9:59 ` David Maximiliano Hermitte
2026-07-07 20:02 ` Viacheslav Dubeyko
2026-07-07 23:28 ` [PATCH v4] " David Maximiliano Hermitte
3 siblings, 1 reply; 11+ messages in thread
From: David Maximiliano Hermitte @ 2026-07-06 9:59 UTC (permalink / raw)
To: Viacheslav Dubeyko, Jori Koolstra
Cc: George Anthony Vernon, Tetsuo Handa, John Paul Adrian Glaubitz,
Yangtao Li, linux-fsdevel, linux-kernel,
syzbot+97e301b4b82ae803d21b, David Maximiliano Hermitte
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 catalog record found by the second lookup before returning
it to callers. Inspect the already-found record with hfs_bnode_read(),
not hfs_brec_read(), and reject records whose CNID is invalid for their
record type or does not match the requested CNID.
Also validate CNIDs in hfs_read_inode() before the inode is populated,
and propagate hfs_read_inode() errors from the resource-fork lookup path.
For the root inode path, require the root catalog record to be a directory
with DirID == HFS_ROOT_CNID, and drop the root inode reference if it was
instantiated as a bad inode.
This keeps hfs_write_inode() unchanged and prevents corrupted catalog
records from reaching the existing reserved-CNID BUG() path during
writeback.
Reported-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
Cc: George Anthony Vernon <contact@gvernon.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: David Maximiliano Hermitte <davemadmaxxx@gmail.com>
---
Changes in v3:
- Drop the stray blank line before assigning the hfs_read_inode() result
in hfs_file_lookup().
Changes in v2:
- Use size_t for the catalog record length.
- Return -EIO when fd->entrylength is larger than hfs_cat_rec instead of
truncating the record length.
- Compare catalog record lengths against sizeof(struct hfs_cat_file) and
sizeof(struct hfs_cat_dir).
- Drop the hfs_is_valid_cnid() call from the found-record CNID
comparison; found_cnid != cnid is enough there.
- Treat non-file/non-directory records found by the second lookup as
invalid for this path.
- Check is_bad_inode() in the resource-fork lookup error path.
- Drop the redundant root DirID check from hfs_fill_super().
fs/hfs/catalog.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
fs/hfs/hfs_fs.h | 19 +++++++++++++++++++
fs/hfs/inode.c | 12 +++++++++---
fs/hfs/super.c | 5 +++++
4 files changed, 76 insertions(+), 4 deletions(-)
diff --git a/fs/hfs/catalog.c b/fs/hfs/catalog.c
index 1bfa36d71e24..50ee709d966f 100644
--- a/fs/hfs/catalog.c
+++ b/fs/hfs/catalog.c
@@ -182,6 +182,43 @@ 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;
+
+ if (fd->entrylength <= 0)
+ return -EIO;
+
+ rec_len = fd->entrylength;
+ if (rec_len > sizeof(rec))
+ 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 +245,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);
}
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);
--
2.43.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3 RESEND] hfs: validate catalog CNIDs before instantiating inodes
2026-07-06 9:59 ` [PATCH v3 RESEND] " David Maximiliano Hermitte
@ 2026-07-07 20:02 ` Viacheslav Dubeyko
[not found] ` <CADy8qZgo3rKwUiPr60tzPhaisMy7R71vjTtPi7+jEXgcarihRw@mail.gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Viacheslav Dubeyko @ 2026-07-07 20:02 UTC (permalink / raw)
To: David Maximiliano Hermitte, Jori Koolstra
Cc: George Anthony Vernon, Tetsuo Handa, John Paul Adrian Glaubitz,
Yangtao Li, linux-fsdevel, linux-kernel,
syzbot+97e301b4b82ae803d21b
On Mon, 2026-07-06 at 09:59 +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 catalog record found by the second lookup before
> returning
> it to callers. Inspect the already-found record with
> hfs_bnode_read(),
> not hfs_brec_read(), and reject records whose CNID is invalid for
> their
> record type or does not match the requested CNID.
>
> Also validate CNIDs in hfs_read_inode() before the inode is
> populated,
> and propagate hfs_read_inode() errors from the resource-fork lookup
> path.
> For the root inode path, require the root catalog record to be a
> directory
> with DirID == HFS_ROOT_CNID, and drop the root inode reference if it
> was
> instantiated as a bad inode.
>
> This keeps hfs_write_inode() unchanged and prevents corrupted catalog
> records from reaching the existing reserved-CNID BUG() path during
> writeback.
>
> Reported-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
> Cc: George Anthony Vernon <contact@gvernon.com>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: David Maximiliano Hermitte <davemadmaxxx@gmail.com>
> ---
> Changes in v3:
> - Drop the stray blank line before assigning the hfs_read_inode()
> result
> in hfs_file_lookup().
>
> Changes in v2:
> - Use size_t for the catalog record length.
> - Return -EIO when fd->entrylength is larger than hfs_cat_rec instead
> of
> truncating the record length.
> - Compare catalog record lengths against sizeof(struct hfs_cat_file)
> and
> sizeof(struct hfs_cat_dir).
> - Drop the hfs_is_valid_cnid() call from the found-record CNID
> comparison; found_cnid != cnid is enough there.
> - Treat non-file/non-directory records found by the second lookup as
> invalid for this path.
> - Check is_bad_inode() in the resource-fork lookup error path.
> - Drop the redundant root DirID check from hfs_fill_super().
>
> fs/hfs/catalog.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
> fs/hfs/hfs_fs.h | 19 +++++++++++++++++++
> fs/hfs/inode.c | 12 +++++++++---
> fs/hfs/super.c | 5 +++++
> 4 files changed, 76 insertions(+), 4 deletions(-)
>
> diff --git a/fs/hfs/catalog.c b/fs/hfs/catalog.c
> index 1bfa36d71e24..50ee709d966f 100644
> --- a/fs/hfs/catalog.c
> +++ b/fs/hfs/catalog.c
> @@ -182,6 +182,43 @@ 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;
> +
> + if (fd->entrylength <= 0)
> + return -EIO;
> +
> + rec_len = fd->entrylength;
> + if (rec_len > sizeof(rec))
> + return -EIO;
Why do not have one check?
rec_len = fd->entrylength;
if (rec_len <= 0 || rec_len > sizeof(rec))
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))
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))
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 +245,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)
Have you decided not to have the hfs_is_valid_cnid() for the case of
hfs_write_inode()?
Thanks,
Slava.
> 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);
> }
> 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);
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 RESEND] hfs: validate catalog CNIDs before instantiating inodes
[not found] ` <CADy8qZgo3rKwUiPr60tzPhaisMy7R71vjTtPi7+jEXgcarihRw@mail.gmail.com>
@ 2026-07-07 21:44 ` Viacheslav Dubeyko
0 siblings, 0 replies; 11+ messages in thread
From: Viacheslav Dubeyko @ 2026-07-07 21:44 UTC (permalink / raw)
To: dave mad maxXx
Cc: Jori Koolstra, George Anthony Vernon, Tetsuo Handa,
John Paul Adrian Glaubitz, Yangtao Li, linux-fsdevel,
linux-kernel, syzbot+97e301b4b82ae803d21b
On Tue, 2026-07-07 at 18:31 -0300, dave mad maxXx wrote:
> Hi Slava,
> Thank you for the review.
> Yes, I agree that the initial length validation can be simplified to:
> rec_len = fd->entrylength;
> if (rec_len <= 0 || rec_len > sizeof(rec))
> return -EIO;
>
> I also agree that, for the fixed-size file and directory catalog
> records, the checks should require the exact record size:
> rec_len != sizeof(struct hfs_cat_file)
>
> and:
> rec_len != sizeof(struct hfs_cat_dir)
>
> Regarding hfs_write_inode(), I intentionally left it unchanged
> because the patch was trying to reject invalid CNIDs at the catalog
> lookup and inode-instantiation boundaries, before a corrupted inode
> could reach writeback.
> However, I agree that an additional validation there may be useful as
> defense in depth. Would you prefer hfs_is_valid_cnid() to be checked
> in hfs_write_inode() as well, and should that check precede the
> existing BUG path while otherwise leaving its behavior unchanged?
> I will prepare the next revision after your guidance on that point.
Let's leave hfs_write_inode() untouched. I don't see a valid point in
this modification.
Thanks,
Slava.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v4] hfs: validate catalog CNIDs before instantiating inodes
2026-07-02 18:38 [PATCH] hfs: validate catalog CNIDs before instantiating inodes David Maximiliano Hermitte
` (2 preceding siblings ...)
2026-07-06 9:59 ` [PATCH v3 RESEND] " David Maximiliano Hermitte
@ 2026-07-07 23:28 ` David Maximiliano Hermitte
2026-07-08 19:44 ` Viacheslav Dubeyko
2026-07-08 21:56 ` [PATCH v5] " David Maximiliano Hermitte
3 siblings, 2 replies; 11+ messages in thread
From: David Maximiliano Hermitte @ 2026-07-07 23:28 UTC (permalink / raw)
To: Viacheslav Dubeyko, Jori Koolstra
Cc: George Anthony Vernon, Tetsuo Handa, John Paul Adrian Glaubitz,
Yangtao Li, linux-fsdevel, linux-kernel,
syzbot+97e301b4b82ae803d21b
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@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
Cc: George Anthony Vernon <contact@gvernon.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: David Maximiliano Hermitte <davemadmaxxx@gmail.com>
---
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))
+ 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);
}
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);
--
2.43.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v4] hfs: validate catalog CNIDs before instantiating inodes
2026-07-07 23:28 ` [PATCH v4] " David Maximiliano Hermitte
@ 2026-07-08 19:44 ` Viacheslav Dubeyko
2026-07-08 21:56 ` [PATCH v5] " David Maximiliano Hermitte
1 sibling, 0 replies; 11+ messages in thread
From: Viacheslav Dubeyko @ 2026-07-08 19:44 UTC (permalink / raw)
To: David Maximiliano Hermitte, Jori Koolstra
Cc: George Anthony Vernon, Tetsuo Handa, John Paul Adrian Glaubitz,
Yangtao Li, linux-fsdevel, linux-kernel,
syzbot+97e301b4b82ae803d21b
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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
> Cc: George Anthony Vernon <contact@gvernon.com>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: David Maximiliano Hermitte <davemadmaxxx@gmail.com>
> ---
> 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);
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v5] hfs: validate catalog CNIDs before instantiating inodes
2026-07-07 23:28 ` [PATCH v4] " David Maximiliano Hermitte
2026-07-08 19:44 ` Viacheslav Dubeyko
@ 2026-07-08 21:56 ` David Maximiliano Hermitte
2026-07-08 22:17 ` Viacheslav Dubeyko
1 sibling, 1 reply; 11+ messages in thread
From: David Maximiliano Hermitte @ 2026-07-08 21:56 UTC (permalink / raw)
To: Viacheslav Dubeyko, Jori Koolstra
Cc: George Anthony Vernon, Tetsuo Handa, John Paul Adrian Glaubitz,
Yangtao Li, linux-fsdevel, linux-kernel,
syzbot+97e301b4b82ae803d21b
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@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
Cc: George Anthony Vernon <contact@gvernon.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: David Maximiliano Hermitte <davemadmaxxx@gmail.com>
---
Changes in v5:
- Keep rec_len signed by using int to match fd->entrylength, making
the non-positive length check meaningful before passing the validated
value to hfs_bnode_read().
- Use (size_t) casts when comparing rec_len against record sizes to
avoid signed/unsigned comparisons.
- Preserve the original error code in hfs_file_lookup(), returning
-EIO separately only when the inode is marked bad.
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 | 13 ++++++++++++-
fs/hfs/super.c | 5 +++++
4 files changed, 76 insertions(+), 2 deletions(-)
diff --git a/fs/hfs/catalog.c b/fs/hfs/catalog.c
index 1bfa36d71e24..490c5e130da3 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;
+ int rec_len;
+
+ rec_len = fd->entrylength;
+ if (rec_len <= 0 || (size_t)rec_len > sizeof(rec))
+ 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 ((size_t)rec_len != sizeof(struct hfs_cat_file))
+ return -EIO;
+ found_cnid = be32_to_cpu(rec.file.FlNum);
+ break;
+ case HFS_CDR_DIR:
+ if ((size_t)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..c9e75c2e576a 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,13 +577,18 @@ 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) {
iput(inode);
return ERR_PTR(res);
}
+
+ if (is_bad_inode(inode)) {
+ iput(inode);
+ return ERR_PTR(-EIO);
+ }
HFS_I(inode)->rsrc_inode = dir;
HFS_I(dir)->rsrc_inode = inode;
igrab(dir);
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);
--
2.43.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v5] hfs: validate catalog CNIDs before instantiating inodes
2026-07-08 21:56 ` [PATCH v5] " David Maximiliano Hermitte
@ 2026-07-08 22:17 ` Viacheslav Dubeyko
2026-07-08 23:09 ` Tetsuo Handa
0 siblings, 1 reply; 11+ messages in thread
From: Viacheslav Dubeyko @ 2026-07-08 22:17 UTC (permalink / raw)
To: David Maximiliano Hermitte, Jori Koolstra
Cc: George Anthony Vernon, Tetsuo Handa, John Paul Adrian Glaubitz,
Yangtao Li, linux-fsdevel, linux-kernel,
syzbot+97e301b4b82ae803d21b
On Wed, 2026-07-08 at 21:56 +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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
> Cc: George Anthony Vernon <contact@gvernon.com>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: David Maximiliano Hermitte <davemadmaxxx@gmail.com>
> ---
> Changes in v5:
> - Keep rec_len signed by using int to match fd->entrylength, making
> the non-positive length check meaningful before passing the
> validated
> value to hfs_bnode_read().
> - Use (size_t) casts when comparing rec_len against record sizes to
> avoid signed/unsigned comparisons.
> - Preserve the original error code in hfs_file_lookup(), returning
> -EIO separately only when the inode is marked bad.
>
> 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 | 13 ++++++++++++-
> fs/hfs/super.c | 5 +++++
> 4 files changed, 76 insertions(+), 2 deletions(-)
>
> diff --git a/fs/hfs/catalog.c b/fs/hfs/catalog.c
> index 1bfa36d71e24..490c5e130da3 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;
> + int rec_len;
> +
> + rec_len = fd->entrylength;
> + if (rec_len <= 0 || (size_t)rec_len > sizeof(rec))
> + 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 ((size_t)rec_len != sizeof(struct hfs_cat_file))
> + return -EIO;
> + found_cnid = be32_to_cpu(rec.file.FlNum);
> + break;
> + case HFS_CDR_DIR:
> + if ((size_t)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..c9e75c2e576a 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,13 +577,18 @@ 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) {
> iput(inode);
> return ERR_PTR(res);
> }
> +
> + if (is_bad_inode(inode)) {
> + iput(inode);
> + return ERR_PTR(-EIO);
> + }
> HFS_I(inode)->rsrc_inode = dir;
> HFS_I(dir)->rsrc_inode = inode;
> igrab(dir);
> 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);
Looks good. Thanks a lot for the fix.
Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>
I am happy to test the patch. But xfstests still has too many failures
for the case of HFS file system.
Thanks,
Slava.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v5] hfs: validate catalog CNIDs before instantiating inodes
2026-07-08 22:17 ` Viacheslav Dubeyko
@ 2026-07-08 23:09 ` Tetsuo Handa
0 siblings, 0 replies; 11+ messages in thread
From: Tetsuo Handa @ 2026-07-08 23:09 UTC (permalink / raw)
To: Viacheslav Dubeyko, David Maximiliano Hermitte, Jori Koolstra
Cc: George Anthony Vernon, John Paul Adrian Glaubitz, Yangtao Li,
linux-fsdevel, linux-kernel, syzbot+97e301b4b82ae803d21b
On 2026/07/09 7:17, Viacheslav Dubeyko wrote:
> Looks good. Thanks a lot for the fix.
Not a new bug introduced by this patch, but sashiko thinks that
there are things to check.
https://sashiko.dev/#/patchset/20260708215636.73815-1-davemadmaxxx%40gmail.com
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2026-07-08 23:09 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-02 18:38 [PATCH] hfs: validate catalog CNIDs before instantiating inodes David Maximiliano Hermitte
2026-07-02 23:28 ` Tetsuo Handa
2026-07-02 23:59 ` Viacheslav Dubeyko
2026-07-06 9:59 ` [PATCH v3 RESEND] " David Maximiliano Hermitte
2026-07-07 20:02 ` Viacheslav Dubeyko
[not found] ` <CADy8qZgo3rKwUiPr60tzPhaisMy7R71vjTtPi7+jEXgcarihRw@mail.gmail.com>
2026-07-07 21:44 ` Viacheslav Dubeyko
2026-07-07 23:28 ` [PATCH v4] " David Maximiliano Hermitte
2026-07-08 19:44 ` Viacheslav Dubeyko
2026-07-08 21:56 ` [PATCH v5] " David Maximiliano Hermitte
2026-07-08 22:17 ` Viacheslav Dubeyko
2026-07-08 23:09 ` Tetsuo Handa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox