Linux filesystem development
 help / color / mirror / Atom feed
* [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