[PATCH v4] hfs: Validate CNIDs in hfs_read_inode

[PATCH v4] hfs: Validate CNIDs in hfs_read_inode

[George Anthony Vernon]

hfs_read_inode previously did not validate CNIDs read from disk, thereby
allowing inodes to be constructed with disallowed CNIDs and placed on
the dirty list, eventually hitting a bug on writeback.

Validate reserved CNIDs as specified for HFS according to
"Inside Macintosh: Files."

This issue was discussed at length on LKML previously, the discussion
is linked below.

Syzbot tested this patch on mainline and the bug did not replicate.
This patch was regression tested by issuing various system calls on a
mounted HFS filesystem and validating that file creation, deletion,
reads and writes all work.

Link: https://lore.kernel.org/all/427fcb57-8424-4e52-9f21-7041b2c4ae5b@
I-love.SAKURA.ne.jp/T/
Reported-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Tested-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
Signed-off-by: George Anthony Vernon <contact@gvernon.com>
---
Changes from V3->V4:
- Remove explicit "do nothing" case statement labels in favor of
  implicit default
- Check superblock for bad inode

Changes from V2->V3:
- Use HFS-specific references in preference to TN1150
- Remove Tetsuo's additional superblock check from patch series
- Rename is_valid_cnid() -> is_valid_catalog_record()
- Add static inline hfs_inode_type() helper function
- Do not BUG() on detected bad inode, use pr_warn()

Changes from V1->V2:
- is_valid_cnid prototype now takes u32 and u8 types
- Add fsck advice in dmesg
- Replace make_bad_inode calls in hfs_read_inode with gotos
- Reuse same check in hfs_write_inode
- Disallow HFS_POR_CNID, HFS_BAD_CNID, and HFS_EXCH_CNID
- Add Tetsuo's patch to mine and make it a patch series

 fs/hfs/inode.c | 65 +++++++++++++++++++++++++++++++++++++++-----------
 fs/hfs/super.c |  2 +-
 2 files changed, 52 insertions(+), 15 deletions(-)

diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index 878535db64d6..469ea6401d47 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -340,6 +340,31 @@ static int hfs_test_inode(struct inode *inode, void *data)
 	}
 }
 
+/*
+ * is_valid_catalog_record
+ *
+ * Validate the CNID of a catalog record
+ */
+static inline
+bool is_valid_catalog_record(u32 cnid, u8 type)
+{
+	if (likely(cnid >= HFS_FIRSTUSER_CNID))
+		return true;
+
+	switch (cnid) {
+	case HFS_ROOT_CNID:
+		return type == HFS_CDR_DIR;
+	case HFS_EXT_CNID:
+	case HFS_CAT_CNID:
+		return type == HFS_CDR_FIL;
+	default:
+		/* Invalid reserved CNID */
+		break;
+	}
+
+	return false;
+}
+
 /*
  * hfs_read_inode
  */
@@ -369,6 +394,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
 	rec = idata->rec;
 	switch (rec->type) {
 	case HFS_CDR_FIL:
+		if (!is_valid_catalog_record(rec->file.FlNum, rec->type))
+			goto make_bad_inode;
 		if (!HFS_IS_RSRC(inode)) {
 			hfs_inode_read_fork(inode, rec->file.ExtRec, rec->file.LgLen,
 					    rec->file.PyLen, be16_to_cpu(rec->file.ClpSize));
@@ -390,6 +417,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
 		inode->i_mapping->a_ops = &hfs_aops;
 		break;
 	case HFS_CDR_DIR:
+		if (!is_valid_catalog_record(rec->dir.DirID, rec->type))
+			goto make_bad_inode;
 		inode->i_ino = be32_to_cpu(rec->dir.DirID);
 		inode->i_size = be16_to_cpu(rec->dir.Val) + 2;
 		HFS_I(inode)->fs_blocks = 0;
@@ -399,8 +428,13 @@ static int hfs_read_inode(struct inode *inode, void *data)
 		inode->i_op = &hfs_dir_inode_operations;
 		inode->i_fop = &hfs_dir_operations;
 		break;
+	make_bad_inode:
+		pr_warn("Invalid inode with cnid %lu\n", inode->i_ino);
+		pr_warn("Volume is probably corrupted, try performing fsck.\n");
+		fallthrough;
 	default:
 		make_bad_inode(inode);
+		break;
 	}
 	return 0;
 }
@@ -448,6 +482,11 @@ void hfs_inode_write_fork(struct inode *inode, struct hfs_extent *ext,
 					 HFS_SB(inode->i_sb)->alloc_blksz);
 }
 
+static inline u8 hfs_inode_type(struct inode *inode)
+{
+	return S_ISDIR(inode->i_mode) ? HFS_CDR_DIR : HFS_CDR_FIL;
+}
+
 int hfs_write_inode(struct inode *inode, struct writeback_control *wbc)
 {
 	struct inode *main_inode = inode;
@@ -460,20 +499,18 @@ int hfs_write_inode(struct inode *inode, struct writeback_control *wbc)
 	if (res)
 		return res;
 
-	if (inode->i_ino < HFS_FIRSTUSER_CNID) {
-		switch (inode->i_ino) {
-		case HFS_ROOT_CNID:
-			break;
-		case HFS_EXT_CNID:
-			hfs_btree_write(HFS_SB(inode->i_sb)->ext_tree);
-			return 0;
-		case HFS_CAT_CNID:
-			hfs_btree_write(HFS_SB(inode->i_sb)->cat_tree);
-			return 0;
-		default:
-			BUG();
-			return -EIO;
-		}
+	if (!is_valid_catalog_record(inode->i_ino, hfs_inode_type(inode)))
+		return -EIO;
+
+	switch (inode->i_ino) {
+	case HFS_EXT_CNID:
+		hfs_btree_write(HFS_SB(inode->i_sb)->ext_tree);
+		return 0;
+	case HFS_CAT_CNID:
+		hfs_btree_write(HFS_SB(inode->i_sb)->cat_tree);
+		return 0;
+	default:
+		break;
 	}
 
 	if (HFS_IS_RSRC(inode))
diff --git a/fs/hfs/super.c b/fs/hfs/super.c
index a4f2a2bfa6d3..060421770147 100644
--- a/fs/hfs/super.c
+++ b/fs/hfs/super.c
@@ -369,7 +369,7 @@ static int hfs_fill_super(struct super_block *sb, struct fs_context *fc)
 	res = -EINVAL;
 	root_inode = hfs_iget(sb, &fd.search_key->cat, &rec);
 	hfs_find_exit(&fd);
-	if (!root_inode)
+	if (!root_inode || is_bad_inode(root_inode))
 		goto bail_no_root;
 
 	set_default_d_op(sb, &hfs_dentry_operations);
-- 
2.53.0

Re: [PATCH v4] hfs: Validate CNIDs in hfs_read_inode

[Tetsuo Handa]

Since is_valid_catalog_record() is called before inode->i_ino is assigned,

+               pr_warn("Invalid inode with cnid %lu\n", inode->i_ino);

always prints 0.

kernel test robot <lkp@intel.com> reported that this patch needs below change.

-               if (!is_valid_catalog_record(rec->file.FlNum, rec->type))
+               if (!is_valid_catalog_record(be32_to_cpu(rec->file.FlNum), rec->type))

-               if (!is_valid_catalog_record(rec->dir.DirID, rec->type))
+               if (!is_valid_catalog_record(be32_to_cpu(rec->dir.DirID), rec->type))

Because of this endian bug, syzbot did not test is_valid_catalog_record() == false case.

This patch also needs below change.

-       if (!root_inode || is_bad_inode(root_inode))
+       if (!root_inode)
                goto bail_no_root;
+       if (is_bad_inode(root_inode)) {
+               iput(root_inode);
+               goto bail_no_root;
+       }

Since this bug is reported when "rmmod hfs" is done, syzbot would not be
able to find this bug.

And even after both changes are applied, my patch still makes sense
because mount() operation still succeeds for cnid >= 16. :-)

RE: [PATCH v4] hfs: Validate CNIDs in hfs_read_inode

[Viacheslav Dubeyko]

On Thu, 2026-03-12 at 19:45 +0900, Tetsuo Handa wrote:
> Since is_valid_catalog_record() is called before inode->i_ino is assigned,
> 
> +               pr_warn("Invalid inode with cnid %lu\n", inode->i_ino);
> 
> always prints 0.

Yeah, exactly.

> 
> kernel test robot <lkp@intel.com> reported that this patch needs below change.
> 
> -               if (!is_valid_catalog_record(rec->file.FlNum, rec->type))
> +               if (!is_valid_catalog_record(be32_to_cpu(rec->file.FlNum), rec->type))
> 
> -               if (!is_valid_catalog_record(rec->dir.DirID, rec->type))
> +               if (!is_valid_catalog_record(be32_to_cpu(rec->dir.DirID), rec->type))
> 
> Because of this endian bug, syzbot did not test is_valid_catalog_record() == false case.

Yes, you are right here.

> 
> This patch also needs below change.
> 
> -       if (!root_inode || is_bad_inode(root_inode))
> +       if (!root_inode)
>                 goto bail_no_root;
> +       if (is_bad_inode(root_inode)) {
> +               iput(root_inode);
> +               goto bail_no_root;
> +       }

Yes, it's good improvement.

> 
> Since this bug is reported when "rmmod hfs" is done, syzbot would not be
> able to find this bug.
> 
> And even after both changes are applied, my patch still makes sense
> because mount() operation still succeeds for cnid >= 16. :-)

I don't follow how it could happen. Please, take a look here [1]:

	/* try to get the root inode */
	res = hfs_find_init(HFS_SB(sb)->cat_tree, &fd);
	if (res)
		goto bail_no_root;
	res = hfs_cat_find_brec(sb, HFS_ROOT_CNID, &fd);
	if (!res) {
		if (fd.entrylength != sizeof(rec.dir)) {
			res =  -EIO;
			goto bail_hfs_find;
		}
		hfs_bnode_read(fd.bnode, &rec, fd.entryoffset, fd.entrylength);
		if (rec.type != HFS_CDR_DIR)
			res = -EIO;
	}
	if (res)
		goto bail_hfs_find;
	res = -EINVAL;
	root_inode = hfs_iget(sb, &fd.search_key->cat, &rec);
	hfs_find_exit(&fd);
	if (!root_inode)
		goto bail_no_root;

We request to find exactly HFS_ROOT_CNID. If root folder has another CNID, then
we simply cannot find the record for root folder in Catalog File. So, we never
will call hfs_iget() if we cannot find the record.

Thanks,
Slava.

[1] https://elixir.bootlin.com/linux/v6.19/source/fs/hfs/super.c#L350

Re: [PATCH v4] hfs: Validate CNIDs in hfs_read_inode

[Tetsuo Handa]

[-- Attachment #1: Type: text/plain, Size: 1565 bytes --]

On 2026/03/13 8:13, Viacheslav Dubeyko wrote:
>> And even after both changes are applied, my patch still makes sense
>> because mount() operation still succeeds for cnid >= 16. :-)
> 
> I don't follow how it could happen. Please, take a look here [1]:

Do you remember that you said

  "Why do not localize the all checks in hfs_read_inode()?"

in https://lkml.kernel.org/r/5498a57ea660b5366ef213acd554aba55a5804d1.camel@ibm.com ?

We came to a patch that validates and calls make_bad_inode() for only 0 <= cnid <= 15 range.
We have never come to a patch that calls make_bad_inode() for cnid >= 16. That is why my
patch is needed; my patch rejects mount() operation if the inode number of the record
retrieved as a result of hfs_cat_find_brec(HFS_ROOT_CNID) is not HFS_ROOT_CNID.

Anyway, seeing is believing. Try testing with the attached reproducer. This reproducer
tries to mount a crafted HFS image on /mnt . Prepare /dev/loop0 in unused state, and
then run the below command line. You will see that mount() succeeds for cnid == 2 and
cnid >= 16.

# for cnid in $(seq 0 20); do unshare -m ./repro $cnid; done 2>/dev/null
cnid=0 mount=-1 (22)
cnid=1 mount=-1 (22)
cnid=2 mount=0 (0)
cnid=3 mount=-1 (20)
cnid=4 mount=-1 (20)
cnid=5 mount=-1 (22)
cnid=6 mount=-1 (22)
cnid=7 mount=-1 (22)
cnid=8 mount=-1 (22)
cnid=9 mount=-1 (22)
cnid=10 mount=-1 (22)
cnid=11 mount=-1 (22)
cnid=12 mount=-1 (22)
cnid=13 mount=-1 (22)
cnid=14 mount=-1 (22)
cnid=15 mount=-1 (22)
cnid=16 mount=0 (0)
cnid=17 mount=0 (0)
cnid=18 mount=0 (0)
cnid=19 mount=0 (0)
cnid=20 mount=0 (0)

[-- Attachment #2: repro.c --]
[-- Type: text/plain, Size: 15780 bytes --]

// https://syzkaller.appspot.com/bug?id=ee595bf9e099fff0610828e37bbbcdb7a2933f58
// autogenerated by syzkaller (https://github.com/google/syzkaller)

#define _GNU_SOURCE

#include <dirent.h>
#include <endian.h>
#include <errno.h>
#include <fcntl.h>
#include <setjmp.h>
#include <signal.h>
#include <stdarg.h>
#include <stdbool.h>
#include <stddef.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/ioctl.h>
#include <sys/mman.h>
#include <sys/mount.h>
#include <sys/prctl.h>
#include <sys/stat.h>
#include <sys/syscall.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <time.h>
#include <unistd.h>

#include <linux/loop.h>

#ifndef __NR_memfd_create
#define __NR_memfd_create 319
#endif

static unsigned int cnid;

//% This code is derived from puff.{c,h}, found in the zlib development. The
//% original files come with the following copyright notice:

//% Copyright (C) 2002-2013 Mark Adler, all rights reserved
//% version 2.3, 21 Jan 2013
//% This software is provided 'as-is', without any express or implied
//% warranty.  In no event will the author be held liable for any damages
//% arising from the use of this software.
//% Permission is granted to anyone to use this software for any purpose,
//% including commercial applications, and to alter it and redistribute it
//% freely, subject to the following restrictions:
//% 1. The origin of this software must not be misrepresented; you must not
//%    claim that you wrote the original software. If you use this software
//%    in a product, an acknowledgment in the product documentation would be
//%    appreciated but is not required.
//% 2. Altered source versions must be plainly marked as such, and must not be
//%    misrepresented as being the original software.
//% 3. This notice may not be removed or altered from any source distribution.
//% Mark Adler    madler@alumni.caltech.edu

//% BEGIN CODE DERIVED FROM puff.{c,h}

#define MAXBITS 15
#define MAXLCODES 286
#define MAXDCODES 30
#define MAXCODES (MAXLCODES + MAXDCODES)
#define FIXLCODES 288

struct puff_state {
	unsigned char* out;
	unsigned long outlen;
	unsigned long outcnt;
	const unsigned char* in;
	unsigned long inlen;
	unsigned long incnt;
	int bitbuf;
	int bitcnt;
	jmp_buf env;
};
static int puff_bits(struct puff_state* s, int need)
{
	long val = s->bitbuf;
	while (s->bitcnt < need) {
		if (s->incnt == s->inlen)
			longjmp(s->env, 1);
		val |= (long)(s->in[s->incnt++]) << s->bitcnt;
		s->bitcnt += 8;
	}
	s->bitbuf = (int)(val >> need);
	s->bitcnt -= need;
	return (int)(val & ((1L << need) - 1));
}
static int puff_stored(struct puff_state* s)
{
	s->bitbuf = 0;
	s->bitcnt = 0;
	if (s->incnt + 4 > s->inlen)
		return 2;
	unsigned len = s->in[s->incnt++];
	len |= s->in[s->incnt++] << 8;
	if (s->in[s->incnt++] != (~len & 0xff) ||
	    s->in[s->incnt++] != ((~len >> 8) & 0xff))
		return -2;
	if (s->incnt + len > s->inlen)
		return 2;
	if (s->outcnt + len > s->outlen)
		return 1;
	for (; len--; s->outcnt++, s->incnt++) {
		if (s->in[s->incnt])
			s->out[s->outcnt] = s->in[s->incnt];
	}
	return 0;
}
struct puff_huffman {
	short* count;
	short* symbol;
};
static int puff_decode(struct puff_state* s, const struct puff_huffman* h)
{
	int first = 0;
	int index = 0;
	int bitbuf = s->bitbuf;
	int left = s->bitcnt;
	int code = first = index = 0;
	int len = 1;
	short* next = h->count + 1;
	while (1) {
		while (left--) {
			code |= bitbuf & 1;
			bitbuf >>= 1;
			int count = *next++;
			if (code - count < first) {
				s->bitbuf = bitbuf;
				s->bitcnt = (s->bitcnt - len) & 7;
				return h->symbol[index + (code - first)];
			}
			index += count;
			first += count;
			first <<= 1;
			code <<= 1;
			len++;
		}
		left = (MAXBITS + 1) - len;
		if (left == 0)
			break;
		if (s->incnt == s->inlen)
			longjmp(s->env, 1);
		bitbuf = s->in[s->incnt++];
		if (left > 8)
			left = 8;
	}
	return -10;
}
static int puff_construct(struct puff_huffman* h, const short* length, int n)
{
	int len;
	for (len = 0; len <= MAXBITS; len++)
		h->count[len] = 0;
	int symbol;
	for (symbol = 0; symbol < n; symbol++)
		(h->count[length[symbol]])++;
	if (h->count[0] == n)
		return 0;
	int left = 1;
	for (len = 1; len <= MAXBITS; len++) {
		left <<= 1;
		left -= h->count[len];
		if (left < 0)
			return left;
	}
	short offs[MAXBITS + 1];
	offs[1] = 0;
	for (len = 1; len < MAXBITS; len++)
		offs[len + 1] = offs[len] + h->count[len];
	for (symbol = 0; symbol < n; symbol++)
		if (length[symbol] != 0)
			h->symbol[offs[length[symbol]]++] = symbol;
	return left;
}
static int puff_codes(struct puff_state* s, const struct puff_huffman* lencode,
                      const struct puff_huffman* distcode)
{
	static const short lens[29] = {3,  4,  5,  6,   7,   8,   9,   10,  11, 13,
				       15, 17, 19, 23,  27,  31,  35,  43,  51, 59,
				       67, 83, 99, 115, 131, 163, 195, 227, 258};
	static const short lext[29] = {0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 2, 2, 2,
				       2, 3, 3, 3, 3, 4, 4, 4, 4, 5, 5, 5, 5, 0};
	static const short dists[30] = {
		1,    2,    3,    4,    5,    7,    9,    13,    17,    25,
		33,   49,   65,   97,   129,  193,  257,  385,   513,   769,
		1025, 1537, 2049, 3073, 4097, 6145, 8193, 12289, 16385, 24577};
	static const short dext[30] = {0, 0, 0,  0,  1,  1,  2,  2,  3,  3,
				       4, 4, 5,  5,  6,  6,  7,  7,  8,  8,
				       9, 9, 10, 10, 11, 11, 12, 12, 13, 13};
	int symbol;
	do {
		symbol = puff_decode(s, lencode);
		if (symbol < 0)
			return symbol;
		if (symbol < 256) {
			if (s->outcnt == s->outlen)
				return 1;
			if (symbol)
				s->out[s->outcnt] = symbol;
			s->outcnt++;
		} else if (symbol > 256) {
			symbol -= 257;
			if (symbol >= 29)
				return -10;
			int len = lens[symbol] + puff_bits(s, lext[symbol]);
			symbol = puff_decode(s, distcode);
			if (symbol < 0)
				return symbol;
			unsigned dist = dists[symbol] + puff_bits(s, dext[symbol]);
			if (dist > s->outcnt)
				return -11;
			if (s->outcnt + len > s->outlen)
				return 1;
			while (len--) {
				if (dist <= s->outcnt && s->out[s->outcnt - dist])
					s->out[s->outcnt] = s->out[s->outcnt - dist];
				s->outcnt++;
			}
		}
	} while (symbol != 256);
	return 0;
}
static int puff_fixed(struct puff_state* s)
{
	static int virgin = 1;
	static short lencnt[MAXBITS + 1], lensym[FIXLCODES];
	static short distcnt[MAXBITS + 1], distsym[MAXDCODES];
	static struct puff_huffman lencode, distcode;
	if (virgin) {
		lencode.count = lencnt;
		lencode.symbol = lensym;
		distcode.count = distcnt;
		distcode.symbol = distsym;
		short lengths[FIXLCODES];
		int symbol;
		for (symbol = 0; symbol < 144; symbol++)
			lengths[symbol] = 8;
		for (; symbol < 256; symbol++)
			lengths[symbol] = 9;
		for (; symbol < 280; symbol++)
			lengths[symbol] = 7;
		for (; symbol < FIXLCODES; symbol++)
			lengths[symbol] = 8;
		puff_construct(&lencode, lengths, FIXLCODES);
		for (symbol = 0; symbol < MAXDCODES; symbol++)
			lengths[symbol] = 5;
		puff_construct(&distcode, lengths, MAXDCODES);
		virgin = 0;
	}
	return puff_codes(s, &lencode, &distcode);
}
static int puff_dynamic(struct puff_state* s)
{
	static const short order[19] = {16, 17, 18, 0, 8,  7, 9,  6, 10, 5,
					11, 4,  12, 3, 13, 2, 14, 1, 15};
	int nlen = puff_bits(s, 5) + 257;
	int ndist = puff_bits(s, 5) + 1;
	int ncode = puff_bits(s, 4) + 4;
	if (nlen > MAXLCODES || ndist > MAXDCODES)
		return -3;
	short lengths[MAXCODES];
	int index;
	for (index = 0; index < ncode; index++)
		lengths[order[index]] = puff_bits(s, 3);
	for (; index < 19; index++)
		lengths[order[index]] = 0;
	short lencnt[MAXBITS + 1], lensym[MAXLCODES];
	struct puff_huffman lencode = {lencnt, lensym};
	int err = puff_construct(&lencode, lengths, 19);
	if (err != 0)
		return -4;
	index = 0;
	while (index < nlen + ndist) {
		int symbol;
		int len;
		symbol = puff_decode(s, &lencode);
		if (symbol < 0)
			return symbol;
		if (symbol < 16)
			lengths[index++] = symbol;
		else {
			len = 0;
			if (symbol == 16) {
				if (index == 0)
					return -5;
				len = lengths[index - 1];
				symbol = 3 + puff_bits(s, 2);
			} else if (symbol == 17)
				symbol = 3 + puff_bits(s, 3);
			else
				symbol = 11 + puff_bits(s, 7);
			if (index + symbol > nlen + ndist)
				return -6;
			while (symbol--)
				lengths[index++] = len;
		}
	}
	if (lengths[256] == 0)
		return -9;
	err = puff_construct(&lencode, lengths, nlen);
	if (err && (err < 0 || nlen != lencode.count[0] + lencode.count[1]))
		return -7;
	short distcnt[MAXBITS + 1], distsym[MAXDCODES];
	struct puff_huffman distcode = {distcnt, distsym};
	err = puff_construct(&distcode, lengths + nlen, ndist);
	if (err && (err < 0 || ndist != distcode.count[0] + distcode.count[1]))
		return -8;
	return puff_codes(s, &lencode, &distcode);
}
static int puff(unsigned char* dest, unsigned long* destlen,
                const unsigned char* source, unsigned long sourcelen)
{
	struct puff_state s = {
		.out = dest,
		.outlen = *destlen,
		.outcnt = 0,
		.in = source,
		.inlen = sourcelen,
		.incnt = 0,
		.bitbuf = 0,
		.bitcnt = 0,
	};
	int err;
	if (setjmp(s.env) != 0)
		err = 2;
	else {
		int last;
		do {
			last = puff_bits(&s, 1);
			int type = puff_bits(&s, 2);
			err = type == 0 ? puff_stored(&s)
				: (type == 1 ? puff_fixed(&s)
                                   : (type == 2 ? puff_dynamic(&s) : -1));
			if (err != 0)
				break;
		} while (!last);
	}
	*destlen = s.outcnt;
	return err;
}

//% END CODE DERIVED FROM puff.{c,h}

#define ZLIB_HEADER_WIDTH 2

static int puff_zlib_to_file(const unsigned char* source,
                             unsigned long sourcelen, int dest_fd)
{
	if (sourcelen < ZLIB_HEADER_WIDTH)
		return 0;
	source += ZLIB_HEADER_WIDTH;
	sourcelen -= ZLIB_HEADER_WIDTH;
	const unsigned long max_destlen = 132 << 20;
	void* ret = mmap(0, max_destlen, PROT_WRITE | PROT_READ,
			 MAP_PRIVATE | MAP_ANON, -1, 0);
	if (ret == MAP_FAILED)
		return -1;
	unsigned char* dest = (unsigned char*)ret;
	unsigned long destlen = max_destlen;
	int err = puff(dest, &destlen, source, sourcelen);
	if (err) {
		munmap(dest, max_destlen);
		errno = -err;
		return -1;
	}
	if (write(dest_fd, dest, destlen) != (ssize_t)destlen) {
		munmap(dest, max_destlen);
		return -1;
	}
	return munmap(dest, max_destlen);
}

static void syz_mount_image(volatile long fsarg, volatile long dir,
                            volatile long flags, volatile long optsarg,
                            volatile long change_dir,
                            volatile unsigned long size, volatile long image)
{
	unsigned char* data = (unsigned char*)image;
	int err = 0, loopfd = -1;
	int memfd = syscall(__NR_memfd_create, "syzkaller", 0);
	if (memfd == -1) {
		err = errno;
		goto error;
	}
	if (puff_zlib_to_file(data, size, memfd)) {
		err = errno;
		goto error_close_memfd;
	}
	loopfd = open("/dev/loop0", O_RDWR);
	if (loopfd == -1) {
		err = errno;
		goto error_close_memfd;
	}
	if (ioctl(loopfd, LOOP_SET_FD, memfd)) {
		if (errno != EBUSY) {
			err = errno;
			goto error_close_loop;
		}
		ioctl(loopfd, LOOP_CLR_FD, 0);
		usleep(1000);
		if (ioctl(loopfd, LOOP_SET_FD, memfd)) {
			err = errno;
			goto error_close_loop;
		}
	}
	pwrite(memfd, &cnid, sizeof(cnid), 4096 + 548);
	close(memfd);
	errno = 0;
	err = mount("/dev/loop0", "/mnt", "hfs", 0, NULL);
	printf("cnid=%d mount=%d (%d)\n", cnid / 0x1000000, err, errno);
	fflush(stdout);
	exit(0);

error_close_loop:
	close(loopfd);
error_close_memfd:
	close(memfd);
error:
	fprintf(stderr, "error %d. Retrying...\n", err);
}

static void execute_one(void)
{
	memcpy((void*)0x200000002c80, "hfs\000", 4);
	memcpy((void*)0x2000000003c0, "./file1\000", 8);
	*(uint8_t*)0x200000000000 = 0;
	sprintf((char*)0x200000000001, "0x%016llx", (long long)-1);
	memcpy(
	       (void*)0x200000000100,
	       "\x78\x9c\xec\xdd\xbf\x6e\xd3\x5c\x18\xc7\xf1\xdf\x71\xd2\x36\xef\x4b\x55"
	       "\xdc\x3f\x08\x09\x31\x15\x2a\x31\xa1\xb6\x0c\x20\x96\x4a\xa8\x77\xc0\xc2"
	       "\x84\x28\x4d\x90\xaa\x5a\x45\x82\x22\xd1\x4e\x81\x19\x71\x01\xec\xdc\x02"
	       "\x17\xc0\xc8\x54\x31\x23\xb1\x31\x31\x30\x16\x96\xa0\x73\x7c\x42\xec\x34"
	       "\x8e\x93\x28\xa9\x93\xf4\xfb\x91\x12\x25\xf6\x79\xec\xe7\xd8\xc7\x3d\x7e"
	       "\x52\xb5\x11\x80\x0b\xeb\xc1\xf6\xf7\x8f\x77\x7e\xd8\x87\x91\x4a\x2a\x49"
	       "\xba\x2f\x05\x92\x2a\x52\x59\xd2\x15\x5d\xad\xbc\x3a\x38\xdc\x3b\x8c\x6a"
	       "\xd5\x6e\x1b\x2a\xb9\x08\xfb\x30\xa5\xac\x36\xbb\x07\xb5\x4e\x8b\x6d\x9c"
	       "\xdb\x97\x17\xda\x77\x65\xcd\x27\x97\x61\x34\x2a\xdf\x54\x2f\x3a\x07\x14"
	       "\xcf\x5d\xfd\x1d\x04\xd2\x9c\xbf\x3a\xdd\xfa\xca\xb9\x67\x36\x1a\x17\x7d"
	       "\xd0\x9b\x53\x9d\xea\xb5\x16\x8a\xce\x03\x00\x50\x2c\x3f\xff\x07\x7e\x9e"
	       "\x9f\x8f\x17\x29\x08\xa4\x35\x3f\xed\xa7\xe7\xff\xc9\x9e\x40\xcd\xaf\x86"
	       "\x53\x74\x1e\x23\xf3\x39\x67\x7d\x62\xfe\x77\x55\x56\xc3\xd8\xf3\x7b\xd9"
	       "\xad\x6a\xd5\x7b\x76\x08\xe8\xae\x0f\x71\x55\xa2\xf4\xa7\xef\x83\x36\xab"
	       "\x78\x64\xa5\x6e\x30\x4d\x5e\x55\xe9\x72\x09\xfe\x7b\xb6\x17\xd5\x6e\xef"
	       "\x3e\x8f\xaa\x81\xde\x6a\xcb\x9b\x69\x35\x5b\x71\xcf\xd5\x78\xe8\x36\x35"
	       "\x6b\x5a\xfb\xfa\xcd\xd9\x4d\xaf\xc6\x1d\xf3\x99\xe5\x4a\x6e\xad\x3f\x97"
	       "\x5c\x1f\x66\x6c\x1f\x36\x83\x93\x56\xfe\x89\x26\xcb\xc3\xdd\x63\x3e\xf3"
	       "\xc5\x9c\x98\xc7\x26\xd4\x07\x55\xff\xdd\xff\x95\x1b\xc6\x1e\x0c\x77\x3c"
	       "\xc2\xb6\x33\x15\xe7\xbf\x9e\xbd\x45\xd7\xcb\x59\xb9\x56\xc9\xb3\x94\x68"
	       "\xb2\xe8\x76\x72\x2d\x7d\xc4\xbb\xf6\xb2\xa4\x8c\x8a\x44\xcd\xf3\xb6\xa8"
	       "\xf4\x07\x04\x61\x5e\x9e\x2e\x6a\xa9\x2d\x2a\xee\xdd\x46\x4e\xd4\x72\xc7"
	       "\xa8\xcd\x9c\xa8\x95\xf6\xa8\xd6\x68\xce\x8e\x1c\x35\xf3\xde\x3c\x32\xab"
	       "\xfa\xa9\x4f\xda\x4e\xdc\xff\x07\xf6\x68\xaf\xa9\x97\x2b\xd3\xb6\x71\x2d"
	       "\xfd\xc8\xe8\xda\x9f\xb2\x6b\x19\xba\xf9\xc4\x5f\x75\xf5\xeb\x1d\x5b\x06"
	       "\x83\xf6\x08\x03\x78\xa7\xa7\xba\xa7\x85\x97\x47\xc7\xfb\x3b\x51\x54\x7b"
	       "\x31\xb5\x2f\xec\x95\x38\x06\x69\x1c\x1d\xef\xff\x6e\x8c\x45\x1a\x3b\x51"
	       "\xd4\x1c\x04\xe3\x92\xcf\xd4\xbe\xb0\x07\xb9\x90\xbd\x37\xe7\x9d\xc1\xb7"
	       "\x53\xd8\x4f\x26\x9c\xa3\xd6\x49\xef\x33\x90\xdf\xcd\x4c\x0b\x7b\xdf\x65"
	       "\xe2\xfa\x2f\x51\xaf\xac\xbb\x9b\x35\xfb\x14\xa6\xef\xd3\xe7\x92\xb1\xb9"
	       "\xb5\x60\x62\x8b\x1b\x19\xb5\xc1\x92\x7b\xfe\x3f\xbb\x82\x4b\x31\x52\x5d"
	       "\x8d\xcc\xca\xa0\xad\xe6\xea\xbc\x47\x57\x73\xdd\xb8\x25\xdd\xec\x65\x8f"
	       "\xb1\xd0\xe7\x39\x7e\xb6\x06\x09\x32\xdb\xfa\xaa\x27\x7c\xfe\x0f\x00\x00"
	       "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x30\x69\x86\xf7\x27\x07"
	       "\x15\x65\xad\x2a\xba\x8f\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
	       "\x00\x00\x00\x00\x00\x4c\xba\x51\x7c\xff\xaf\xff\xef\xf0\xe6\x4c\x9b\x5e"
	       "\xbe\xff\xf7\xa1\xe2\x77\x65\xcd\x6b\x66\x18\x3d\x04\x90\xe5\x6f\x00\x00"
	       "\x00\xff\xff\x7b\x54\x80\x16",
	       673);
	syz_mount_image(/*fs=*/0x200000002c80, /*dir=*/0x2000000003c0,
			/*flags=MS_REC|MS_NOATIME|MS_DIRSYNC|0x200*/ 0x4680,
			/*opts=*/0x200000000000, /*chdir=*/0xfd, /*size=*/0x2a1,
			/*img=*/0x200000000100);
}

int main(int argc, char *argv[])
{
	if (argc == 2)
		cnid = (atoi(argv[1]) & 255) * 0x1000000;
	syscall(__NR_mmap, /*addr=*/0x1ffffffff000ul, /*len=*/0x1000ul, /*prot=*/0ul,
		/*flags=MAP_FIXED|MAP_ANONYMOUS|MAP_PRIVATE*/ 0x32ul,
		/*fd=*/(intptr_t)-1, /*offset=*/0ul);
	syscall(__NR_mmap, /*addr=*/0x200000000000ul, /*len=*/0x1000000ul,
		/*prot=PROT_WRITE|PROT_READ|PROT_EXEC*/ 7ul,
		/*flags=MAP_FIXED|MAP_ANONYMOUS|MAP_PRIVATE*/ 0x32ul,
		/*fd=*/(intptr_t)-1, /*offset=*/0ul);
	syscall(__NR_mmap, /*addr=*/0x200001000000ul, /*len=*/0x1000ul, /*prot=*/0ul,
		/*flags=MAP_FIXED|MAP_ANONYMOUS|MAP_PRIVATE*/ 0x32ul,
		/*fd=*/(intptr_t)-1, /*offset=*/0ul);
	while (1) {
		execute_one();
		sleep(1);
	}
	return 0;
}

RE: [PATCH v4] hfs: Validate CNIDs in hfs_read_inode

[Viacheslav Dubeyko]

On Fri, 2026-03-13 at 20:03 +0900, Tetsuo Handa wrote:
> On 2026/03/13 8:13, Viacheslav Dubeyko wrote:
> > > And even after both changes are applied, my patch still makes sense
> > > because mount() operation still succeeds for cnid >= 16. :-)
> > 
> > I don't follow how it could happen. Please, take a look here [1]:
> 
> Do you remember that you said
> 
>   "Why do not localize the all checks in hfs_read_inode()?"
> 

If we still have not correct implementation of hfs_read_inode() flow, then we
can make it right. The patch is under review yet and it can be changed.

Thanks,
Slava.

Re: [PATCH v4] hfs: Validate CNIDs in hfs_read_inode

[Tetsuo Handa]

On 2026/03/14 3:40, Viacheslav Dubeyko wrote:
> On Fri, 2026-03-13 at 20:03 +0900, Tetsuo Handa wrote:
>> On 2026/03/13 8:13, Viacheslav Dubeyko wrote:
>>>> And even after both changes are applied, my patch still makes sense
>>>> because mount() operation still succeeds for cnid >= 16. :-)
>>>
>>> I don't follow how it could happen. Please, take a look here [1]:
>>
>> Do you remember that you said
>>
>>   "Why do not localize the all checks in hfs_read_inode()?"
>>
> 
> If we still have not correct implementation of hfs_read_inode() flow, then we
> can make it right. The patch is under review yet and it can be changed.
> 

Did you read relevant commit in my patch's description? What you have been
failing to understand is that the value filled in by hfs_bnode_read() can be
corrupted, for the value is given from relevant offset of the file system image.

>>> We request to find exactly HFS_ROOT_CNID. If root folder has another CNID, then
>>> we simply cannot find the record for root folder in Catalog File. So, we never
>>> will call hfs_iget() if we cannot find the record.

Your assumption is absolutely wrong. We request to find HFS_ROOT_CNID, but
there is no guarantee that the CNID value (the value of rec.dir.DirID if
rec.type is HFS_CDR_DIR, or the value of rec->file.FlNum if rec->type is
HFS_CDR_FIL) which is filled in by hfs_bnode_read() is actually HFS_ROOT_CNID,
for the CNID value is nothing but 32bits read from relevant offset of the
file system image.

Since we are passing wrong CNID to hfs_iget(), we can't fix this problem
by updating hfs_read_inode() flow. The correct fix for this report is
"don't call hfs_iget() if CNID is wrong". Your patch can be applies as
a further improvement rather than a fix for this report.

RE: [PATCH v4] hfs: Validate CNIDs in hfs_read_inode

[Viacheslav Dubeyko]

On Sat, 2026-03-14 at 15:35 +0900, Tetsuo Handa wrote:
> On 2026/03/14 3:40, Viacheslav Dubeyko wrote:
> > On Fri, 2026-03-13 at 20:03 +0900, Tetsuo Handa wrote:
> > > On 2026/03/13 8:13, Viacheslav Dubeyko wrote:
> > > > > And even after both changes are applied, my patch still makes sense
> > > > > because mount() operation still succeeds for cnid >= 16. :-)
> > > > 
> > > > I don't follow how it could happen. Please, take a look here [1]:
> > > 
> > > Do you remember that you said
> > > 
> > >   "Why do not localize the all checks in hfs_read_inode()?"
> > > 
> > 
> > If we still have not correct implementation of hfs_read_inode() flow, then we
> > can make it right. The patch is under review yet and it can be changed.
> > 
> 
> Did you read relevant commit in my patch's description? What you have been
> failing to understand is that the value filled in by hfs_bnode_read() can be
> corrupted, for the value is given from relevant offset of the file system image.
> 
> > > > We request to find exactly HFS_ROOT_CNID. If root folder has another CNID, then
> > > > we simply cannot find the record for root folder in Catalog File. So, we never
> > > > will call hfs_iget() if we cannot find the record.
> 
> Your assumption is absolutely wrong. We request to find HFS_ROOT_CNID, but
> there is no guarantee that the CNID value (the value of rec.dir.DirID if
> rec.type is HFS_CDR_DIR, or the value of rec->file.FlNum if rec->type is
> HFS_CDR_FIL) which is filled in by hfs_bnode_read() is actually HFS_ROOT_CNID,
> for the CNID value is nothing but 32bits read from relevant offset of the
> file system image.
> 
> Since we are passing wrong CNID to hfs_iget(), we can't fix this problem
> by updating hfs_read_inode() flow. The correct fix for this report is
> "don't call hfs_iget() if CNID is wrong". Your patch can be applies as
> a further improvement rather than a fix for this report.

It means that, maybe, we need to add the logic of checking the extracted record
by hfs_cat_find_brec(). But I cannot imagine that this logic can extract the
record  with incorrect CNID. Because, it is the main goal of hfs_cat_find_brec()
logic to extract the record that contains requested CNID. And if we requested
the HFS_ROOT_CNID, then this logic should return the record with exactly
requested CNID or return the error code if such record has not been found.

Thanks,
Slava.

Re: [PATCH v4] hfs: Validate CNIDs in hfs_read_inode

[George Anthony Vernon]

Sorry I struggled to understand you here Slava, there's a little bit lost in
translation I think.

On Mon, Mar 16, 2026 at 09:50:14PM +0000, Viacheslav Dubeyko wrote:
> by hfs_cat_find_brec(). But I cannot imagine that this logic can extract the
> record  with incorrect CNID. Because, it is the main goal of hfs_cat_find_brec()
> logic to extract the record that contains requested CNID. And if we requested

Do you mean that you do not think hfs_cat_find_brec *can* return a
record with incorrect CNID, or that you do not think it *should*?

I think Tetsuo is right that hfs_cat_find_brec() will return a catalog
record with different CNID in case of a malformed thread record.

On Mon, Mar 16, 2026 at 09:50:14PM +0000, Viacheslav Dubeyko wrote:
> logic to extract the record that contains requested CNID. And if we requested
> the HFS_ROOT_CNID, then this logic should return the record with exactly
> requested CNID or return the error code if such record has not been found.

Do you mean that hfs_cat_find_brec() should validate the CNID of the
catalog record found by hfs_brec_find()? I'm worried that validating
every B-tree lookup is going to be expensive. We could do it, however.

Thanks,

George

RE: [PATCH v4] hfs: Validate CNIDs in hfs_read_inode

[Viacheslav Dubeyko]

On Wed, 2026-03-18 at 00:37 +0000, George Anthony Vernon wrote:
> Sorry I struggled to understand you here Slava, there's a little bit lost in
> translation I think.
> 
> On Mon, Mar 16, 2026 at 09:50:14PM +0000, Viacheslav Dubeyko wrote:
> > by hfs_cat_find_brec(). But I cannot imagine that this logic can extract the
> > record  with incorrect CNID. Because, it is the main goal of hfs_cat_find_brec()
> > logic to extract the record that contains requested CNID. And if we requested
> 
> Do you mean that you do not think hfs_cat_find_brec *can* return a
> record with incorrect CNID, or that you do not think it *should*?
> 
> I think Tetsuo is right that hfs_cat_find_brec() will return a catalog
> record with different CNID in case of a malformed thread record.
> 
> On Mon, Mar 16, 2026 at 09:50:14PM +0000, Viacheslav Dubeyko wrote:
> > logic to extract the record that contains requested CNID. And if we requested
> > the HFS_ROOT_CNID, then this logic should return the record with exactly
> > requested CNID or return the error code if such record has not been found.
> 
> Do you mean that hfs_cat_find_brec() should validate the CNID of the
> catalog record found by hfs_brec_find()? I'm worried that validating
> every B-tree lookup is going to be expensive. We could do it, however.
> 
> 

If you need to initialize the inode, then you need to find a file or a folder
record in Catalog File (b-tree). It means that there are two possible ways: (1)
find it by name, (2) find it by CNID.

If you know the name only, then you need to find a thread record by name. The
thread record contains associated CNID that can be used to find the final
file/folder record. It means that the second step is the searching the record by
using the CNID. If CNID is OK, then we can find the record. If it is not OK,
then we can find nothing or wrong record.

If we know CNID, then we can try to find the record by CNID directly. We will
fail to find the record if there is no record with such CNID. But you don't need
in thread record in the case of having CNID for the search.

Thanks,
Slava.

Re: [PATCH v4] hfs: Validate CNIDs in hfs_read_inode

[George Anthony Vernon]

On Wed, Mar 18, 2026 at 02:02:10AM +0000, Viacheslav Dubeyko wrote:
> On Wed, 2026-03-18 at 00:37 +0000, George Anthony Vernon wrote:
> > Sorry I struggled to understand you here Slava, there's a little bit lost in
> > translation I think.
> > 
> > On Mon, Mar 16, 2026 at 09:50:14PM +0000, Viacheslav Dubeyko wrote:
> > > by hfs_cat_find_brec(). But I cannot imagine that this logic can extract the
> > > record  with incorrect CNID. Because, it is the main goal of hfs_cat_find_brec()
> > > logic to extract the record that contains requested CNID. And if we requested
> > 
> > Do you mean that you do not think hfs_cat_find_brec *can* return a
> > record with incorrect CNID, or that you do not think it *should*?
> > 
> > I think Tetsuo is right that hfs_cat_find_brec() will return a catalog
> > record with different CNID in case of a malformed thread record.
> > 
> > On Mon, Mar 16, 2026 at 09:50:14PM +0000, Viacheslav Dubeyko wrote:
> > > logic to extract the record that contains requested CNID. And if we requested
> > > the HFS_ROOT_CNID, then this logic should return the record with exactly
> > > requested CNID or return the error code if such record has not been found.
> > 
> > Do you mean that hfs_cat_find_brec() should validate the CNID of the
> > catalog record found by hfs_brec_find()? I'm worried that validating
> > every B-tree lookup is going to be expensive. We could do it, however.
> > 
> > 
> 
> If you need to initialize the inode, then you need to find a file or a folder
> record in Catalog File (b-tree). It means that there are two possible ways: (1)
> find it by name, (2) find it by CNID.
> 
> If you know the name only, then you need to find a thread record by name. The
> thread record contains associated CNID that can be used to find the final
> file/folder record. It means that the second step is the searching the record by
> using the CNID. If CNID is OK, then we can find the record. If it is not OK,
> then we can find nothing or wrong record.
> 
> If we know CNID, then we can try to find the record by CNID directly. We will
> fail to find the record if there is no record with such CNID. But you don't need
> in thread record in the case of having CNID for the search.
> 

I don't think this is entirely right because we can't find a catalog
record by CNID directly. We also need its node name which is contained
in the thread record.

Knowing the CNID of the catalog entry we want to find allows us to
construct a key for the thread record, which we find and read. The
thread record tells us the CNID of the catalog record's parent and the
catalog record's file name, which we combine to create a key for the
catalog record. If the thread record is corrupted, the key we
constructed may find us a different catalog entry with different CNID.

- hfs_cat_find_brec() builds a key for the root CNID thread record [1]
- The contents of the thread record are read into an hfs_cat_rec by
hfs_brec_read(), which calls into hfs_bnode_read [2]
- hfs_cat_find_brec() then populates the key for the root CNID catalog
  record, using the fields of the thread record file [3]
- Finally we can search for the catalog record, and this will populate a
  new hfs_cat_rec [4]

So if the catalog thread record is corrupted, the bug Tetsuo described
can currently happen. The catalog record read from disk may have a
different CNID than the one we wanted to find provided to
hfs_cat_find_brec() hence the need to check the CNID of the catalog file
record returned by hfs_bnode_read in hfs_fill_super.

Therefore Tetsuo's patch to check the returned CNID after calling
hfs_cat_find_brec() seems correct to me, but I think it would be better
do that check inside hfs_cat_find_brec() before it returns, then as you
said the function will return the record with "exactly requested CNID or
return the error code if such record has not been found."

Tetsuo, what do you think about doing your check inside
hfs_cat_find_brec? Something a bit like this:

diff --git a/fs/hfs/catalog.c b/fs/hfs/catalog.c
index b80ba40e3877..5f2b4e223e68 100644
--- a/fs/hfs/catalog.c
+++ b/fs/hfs/catalog.c
@@ -185,10 +185,11 @@ int hfs_cat_keycmp(const btree_key *key1, const btree_key *key2)
 /* Try to get a catalog entry for given catalog id */
 // move to read_super???
 int hfs_cat_find_brec(struct super_block *sb, u32 cnid,
-                     struct hfs_find_data *fd)
+                     struct hfs_find_data *fd, hfs_cat_rec *rec_out)
 {
        hfs_cat_rec rec;
        int res, len, type;
+       u32 cat_cnid;

        hfs_cat_build_key(sb, fd->search_key, cnid, NULL);
        res = hfs_brec_read(fd, &rec, sizeof(rec));
@@ -208,7 +209,37 @@ 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;
+
+       if (fd->entrylength > sizeof(rec)) {
+               pr_err("Bad catalog entry size\n");
+               return -EIO;
+       }
+       hfs_bnode_read(fd->bnode, &rec, fd->entryoffset, fd->entrylength);
+       type = rec.type;
+       switch (type) {
+       case HFS_CDR_FIL:
+               cat_cnid = be32_to_cpu(rec.file.FlNum);
+               break;
+       case HFS_CDR_DIR:
+               cat_cnid = be32_to_cpu(rec.dir.DirID);
+               break;
+       default:
+               pr_err("Bad catalog record type\n");
+               pr_err("Filesystem may be corrupted, try performing fsck\n");
+               return -EIO;
+       }
+
+       if (cat_cnid != cnid) {
+               pr_err("Bad catalog record CNID %u, expected %u\n", cat_cnid, cnid);
+               pr_err("Volume is probably corrupted, try performing fsck.\n");
+               return -EIO;
+       }
+
+       *rec_out = rec;
+       return 0;
 }

 static inline


Thanks,

George

[1] https://elixir.bootlin.com/linux/v7.0-rc4/source/fs/hfs/catalog.c#L193
[2] https://elixir.bootlin.com/linux/v7.0-rc4/source/fs/hfs/bfind.c#L179
[3] https://elixir.bootlin.com/linux/v7.0-rc4/source/fs/hfs/catalog.c#L204
[4] https://elixir.bootlin.com/linux/v7.0-rc4/source/fs/hfs/catalog.c#L211

Re: [PATCH v4] hfs: Validate CNIDs in hfs_read_inode

[Tetsuo Handa]

On 2026/03/19 7:49, George Anthony Vernon wrote:
> Tetsuo, what do you think about doing your check inside
> hfs_cat_find_brec? Something a bit like this:

Currently hfs_cat_find_brec() is called by only hfs_fill_super()
with cnid == HFS_ROOT_CNID. Are you planning to add more callers?

If no, my patch

  -		if (rec.type != HFS_CDR_DIR)
  +		if (rec.type != HFS_CDR_DIR || rec.dir.DirID != cpu_to_be32(HFS_ROOT_CNID))

is sufficient.

If yes, your

> +       if (fd->entrylength > sizeof(rec)) {
> +               pr_err("Bad catalog entry size\n");
> +               return -EIO;
> +       }

part is not compatible with https://elixir.bootlin.com/linux/v7.0-rc4/source/fs/hfs/super.c#L359
which requires that fd->entrylength is exactly 70 bytes in order to make sure that rec.dir is
fully initialized.

  sizeof(struct hfs_cat_file) = 102
  sizeof(struct hfs_cat_dir) = 70
  sizeof(struct hfs_cat_thread) = 46

Even if you are planning to add more callers, my patch will be easier to apply to stable.

Re: [PATCH v4] hfs: Validate CNIDs in hfs_read_inode

[George Vernon]

March 19, 2026 at 9:57 AM, "Tetsuo Handa" <penguin-kernel@i-love.sakura.ne.jp mailto:penguin-kernel@i-love.sakura.ne.jp?to=%22Tetsuo%20Handa%22%20%3Cpenguin-kernel%40i-love.sakura.ne.jp%3E > wrote:


> 
> On 2026/03/19 7:49, George Anthony Vernon wrote:
> 
> > 
> > Tetsuo, what do you think about doing your check inside
> >  hfs_cat_find_brec? Something a bit like this:
> > 
> Currently hfs_cat_find_brec() is called by only hfs_fill_super()
> with cnid == HFS_ROOT_CNID. Are you planning to add more callers?
> 
> If no, my patch
> 
>  - if (rec.type != HFS_CDR_DIR)
>  + if (rec.type != HFS_CDR_DIR || rec.dir.DirID != cpu_to_be32(HFS_ROOT_CNID))
> 
> is sufficient.

I'm trying to write a patch Slava will accept, maybe I didn't fully understand what he wanted but I think he wanted to fix hfs_cat_find_brec() so it should not return an incorrect cat record.

> 
> If yes, your
> 
> > 
> > + if (fd->entrylength > sizeof(rec)) {
> >  + pr_err("Bad catalog entry size\n");
> >  + return -EIO;
> >  + }
> > 
> part is not compatible with https://elixir.bootlin.com/linux/v7.0-rc4/source/fs/hfs/super.c#L359
> which requires that fd->entrylength is exactly 70 bytes in order to make sure that rec.dir is
> fully initialized.
> 
>  sizeof(struct hfs_cat_file) = 102
>  sizeof(struct hfs_cat_dir) = 70
>  sizeof(struct hfs_cat_thread) = 46
> 
This can be changed.
> Even if you are planning to add more callers, my patch will be easier to apply to stable.
>
Yes your patch will be easier to backport, but then again HFS has had minimal churn in recent years, so I think this version will still be simple enough, only maybe the interface for debug prints has changed that I can think of.

Thanks,

George

Re: [PATCH v4] hfs: Validate CNIDs in hfs_read_inode

[Tetsuo Handa]

On 2026/03/19 21:26, George Vernon wrote:
> March 19, 2026 at 9:57 AM, "Tetsuo Handa" <penguin-kernel@i-love.sakura.ne.jp mailto:penguin-kernel@i-love.sakura.ne.jp?to=%22Tetsuo%20Handa%22%20%3Cpenguin-kernel%40i-love.sakura.ne.jp%3E > wrote:
> 
> 
>>
>> On 2026/03/19 7:49, George Anthony Vernon wrote:
>>
>>>
>>> Tetsuo, what do you think about doing your check inside
>>>  hfs_cat_find_brec? Something a bit like this:
>>>
>> Currently hfs_cat_find_brec() is called by only hfs_fill_super()
>> with cnid == HFS_ROOT_CNID. Are you planning to add more callers?
>>
>> If no, my patch
>>
>>  - if (rec.type != HFS_CDR_DIR)
>>  + if (rec.type != HFS_CDR_DIR || rec.dir.DirID != cpu_to_be32(HFS_ROOT_CNID))
>>
>> is sufficient.
> 
> I'm trying to write a patch Slava will accept, maybe I didn't fully understand
> what he wanted but I think he wanted to fix hfs_cat_find_brec() so it should not
> return an incorrect cat record.

Neither do I understand. Is the diff below what Viacheslav Dubeyko wants?

 fs/hfs/catalog.c |   26 ++++++++++++++++----------
 fs/hfs/hfs_fs.h  |    4 ++--
 fs/hfs/super.c   |   11 +----------
 3 files changed, 19 insertions(+), 22 deletions(-)

diff --git a/fs/hfs/catalog.c b/fs/hfs/catalog.c
index 7f5339ee57c1..e24fe49d5850 100644
--- a/fs/hfs/catalog.c
+++ b/fs/hfs/catalog.c
@@ -184,31 +184,37 @@ int hfs_cat_keycmp(const btree_key *key1, const btree_key *key2)
 
 /* Try to get a catalog entry for given catalog id */
 // move to read_super???
-int hfs_cat_find_brec(struct super_block *sb, u32 cnid,
-		      struct hfs_find_data *fd)
+int hfs_cat_find_root_brec(struct super_block *sb, struct hfs_find_data *fd, hfs_cat_rec *rec)
 {
-	hfs_cat_rec rec;
 	int res, len, type;
 
-	hfs_cat_build_key(sb, fd->search_key, cnid, NULL);
-	res = hfs_brec_read(fd, &rec, sizeof(rec));
+	hfs_cat_build_key(sb, fd->search_key, HFS_ROOT_CNID, NULL);
+	res = hfs_brec_read(fd, rec, sizeof(*rec));
 	if (res)
 		return res;
 
-	type = rec.type;
+	type = rec->type;
 	if (type != HFS_CDR_THD && type != HFS_CDR_FTH) {
 		pr_err("found bad thread record in catalog\n");
 		return -EIO;
 	}
 
-	fd->search_key->cat.ParID = rec.thread.ParID;
-	len = fd->search_key->cat.CName.len = rec.thread.CName.len;
+	fd->search_key->cat.ParID = rec->thread.ParID;
+	len = fd->search_key->cat.CName.len = rec->thread.CName.len;
 	if (len > HFS_NAMELEN) {
 		pr_err("bad catalog namelength\n");
 		return -EIO;
 	}
-	memcpy(fd->search_key->cat.CName.name, rec.thread.CName.name, len);
-	return hfs_brec_find(fd);
+	memcpy(fd->search_key->cat.CName.name, rec->thread.CName.name, len);
+	res = hfs_brec_find(fd);
+	if (res)
+		return res;
+	if (fd->entrylength != sizeof(rec->dir))
+		return -EIO;
+	hfs_bnode_read(fd->bnode, rec, fd->entryoffset, sizeof(rec->dir));
+	if (rec->type != HFS_CDR_DIR || rec->dir.DirID != cpu_to_be32(HFS_ROOT_CNID))
+		return -EIO;
+	return 0;
 }
 
 static inline
diff --git a/fs/hfs/hfs_fs.h b/fs/hfs/hfs_fs.h
index ac0e83f77a0f..46641b60913f 100644
--- a/fs/hfs/hfs_fs.h
+++ b/fs/hfs/hfs_fs.h
@@ -145,8 +145,8 @@ extern int hfs_clear_vbm_bits(struct super_block *sb, u16 start, u16 count);
 /* catalog.c */
 extern int hfs_cat_keycmp(const btree_key *key1, const btree_key *key2);
 struct hfs_find_data;
-extern int hfs_cat_find_brec(struct super_block *sb, u32 cnid,
-			     struct hfs_find_data *fd);
+extern int hfs_cat_find_root_brec(struct super_block *sb, struct hfs_find_data *fd,
+				  hfs_cat_rec *rec);
 extern int hfs_cat_create(u32 cnid, struct inode *dir,
 			  const struct qstr *str, struct inode *inode);
 extern int hfs_cat_delete(u32 cnid, struct inode *dir, const struct qstr *str);
diff --git a/fs/hfs/super.c b/fs/hfs/super.c
index 2e52acf282b0..c0be872d4079 100644
--- a/fs/hfs/super.c
+++ b/fs/hfs/super.c
@@ -354,16 +354,7 @@ static int hfs_fill_super(struct super_block *sb, struct fs_context *fc)
 	res = hfs_find_init(HFS_SB(sb)->cat_tree, &fd);
 	if (res)
 		goto bail_no_root;
-	res = hfs_cat_find_brec(sb, HFS_ROOT_CNID, &fd);
-	if (!res) {
-		if (fd.entrylength != sizeof(rec.dir)) {
-			res =  -EIO;
-			goto bail_hfs_find;
-		}
-		hfs_bnode_read(fd.bnode, &rec, fd.entryoffset, fd.entrylength);
-		if (rec.type != HFS_CDR_DIR || rec.dir.DirID != cpu_to_be32(HFS_ROOT_CNID))
-			res = -EIO;
-	}
+	res = hfs_cat_find_root_brec(sb, &fd, &rec);
 	if (res)
 		goto bail_hfs_find;
 	res = -EINVAL;

Re: [PATCH v4] hfs: Validate CNIDs in hfs_read_inode

[Tetsuo Handa]

On 2026/03/17 6:50, Viacheslav Dubeyko wrote:
> It means that, maybe, we need to add the logic of checking the extracted record
> by hfs_cat_find_brec(). But I cannot imagine that this logic can extract the
> record  with incorrect CNID. Because, it is the main goal of hfs_cat_find_brec()
> logic to extract the record that contains requested CNID. And if we requested
> the HFS_ROOT_CNID, then this logic should return the record with exactly
> requested CNID or return the error code if such record has not been found.

I can't understand what you are talking about. Since hfs_cat_find_brec() is called
by only hfs_fill_super() with cnid == HFS_ROOT_CNID, we could remove cnid argument
 from hfs_cat_find_brec() and rename hfs_cat_find_brec() to something else.

What we should talk about is not hfs_cat_find_brec() but hfs_bnode_read().

Although hfs_write_inode() does nothing if the "rec" contained unexpected values

  https://elixir.bootlin.com/linux/v7.0-rc4/source/fs/hfs/inode.c#L499
  https://elixir.bootlin.com/linux/v7.0-rc4/source/fs/hfs/inode.c#L522

, it seems that the "check if we can read requested length" => "read the requested
size" => "check if the read data is appropriate" pattern is the expected
hfs_bnode_read() usage. Therefore, I consider that doing the same pattern for
hfs_fill_super() is reasonable.

  https://elixir.bootlin.com/linux/v7.0-rc4/source/fs/hfs/super.c#L363

Re: [PATCH v4] hfs: Validate CNIDs in hfs_read_inode

[George Anthony Vernon]

On Thu, Mar 12, 2026 at 07:45:17PM +0900, Tetsuo Handa wrote:
> Since is_valid_catalog_record() is called before inode->i_ino is assigned,
> 
> +               pr_warn("Invalid inode with cnid %lu\n", inode->i_ino);
> 
> always prints 0.
> 
Thank you, I will include a fix in the next patch version.
> kernel test robot <lkp@intel.com> reported that this patch needs below change.
> 
> -               if (!is_valid_catalog_record(rec->file.FlNum, rec->type))
> +               if (!is_valid_catalog_record(be32_to_cpu(rec->file.FlNum), rec->type))
> 
> -               if (!is_valid_catalog_record(rec->dir.DirID, rec->type))
> +               if (!is_valid_catalog_record(be32_to_cpu(rec->dir.DirID), rec->type))
> 
> Because of this endian bug, syzbot did not test is_valid_catalog_record() == false case.
> 
Sorry I don't follow this. How can you tell syzbot did not test the case?
> This patch also needs below change.
> 
> -       if (!root_inode || is_bad_inode(root_inode))
> +       if (!root_inode)
>                 goto bail_no_root;
> +       if (is_bad_inode(root_inode)) {
> +               iput(root_inode);
> +               goto bail_no_root;
> +       }
> 
> Since this bug is reported when "rmmod hfs" is done, syzbot would not be
> able to find this bug.
> 
> And even after both changes are applied, my patch still makes sense
> because mount() operation still succeeds for cnid >= 16. :-)
I agree your patch fixes a real bug. I think Slava is suggesting a fix
to hfs_cat_find_brec() so that it validates the catalog records it
returns. Let's continue this discussion in reply to his email if that's
okay.

Thanks,

George

Re: [PATCH v4] hfs: Validate CNIDs in hfs_read_inode

[Tetsuo Handa]

On 2026/03/18 9:10, George Anthony Vernon wrote:
>> Because of this endian bug, syzbot did not test is_valid_catalog_record() == false case.
>>
> Sorry I don't follow this. How can you tell syzbot did not test the case?

Since is_valid_catalog_record() returns "true" for 0 < cnid < 15 range,
hfs_read_inode() was not validating the cnid which the reproducer would
have hit BUG() in hfs_write_inode(). And the reproducer did not hit BUG()
because BUG() in hfs_write_inode() was removed.

---------- (Run this program on a little endian machine like x86_64.)
#include <stdio.h>
#include <arpa/inet.h>

static int is_valid_catalog_record(unsigned int cnid, unsigned char type)
{
        if (cnid >= 16)
                return 1;
        printf("validate %u\n", cnid);
        return 0;
}

int main(int argc, char *argv[])
{
        unsigned int i;

        printf("wrong order\n");
        for (i = 0; i < 20; i++)
                is_valid_catalog_record(htonl(i), 0);
        printf("correct order\n");
        for (i = 0; i < 20; i++)
                is_valid_catalog_record(i, 0);
        return 0;
}
----------

----------
wrong order
validate 0
correct order
validate 0
validate 1
validate 2
validate 3
validate 4
validate 5
validate 6
validate 7
validate 8
validate 9
validate 10
validate 11
validate 12
validate 13
validate 14
validate 15
----------

Re: [PATCH v4] hfs: Validate CNIDs in hfs_read_inode

[Viacheslav Dubeyko]

On Wed, 2026-03-11 at 21:13 +0000, George Anthony Vernon wrote:
> hfs_read_inode previously did not validate CNIDs read from disk, thereby
> allowing inodes to be constructed with disallowed CNIDs and placed on
> the dirty list, eventually hitting a bug on writeback.
> 
> Validate reserved CNIDs as specified for HFS according to
> "Inside Macintosh: Files."
> 
> This issue was discussed at length on LKML previously, the discussion
> is linked below.
> 
> Syzbot tested this patch on mainline and the bug did not replicate.
> This patch was regression tested by issuing various system calls on a
> mounted HFS filesystem and validating that file creation, deletion,
> reads and writes all work.
> 
> Link: https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_all_427fcb57-2D8424-2D4e52-2D9f21-2D7041b2c4ae5b-40&d=DwIDAg&c=BSDicqBQBDjDI9RkVyTcHQ&r=q5bIm4AXMzc8NJu1_RGmnQ2fMWKq4Y4RAkElvUgSs00&m=hbRXIRbizwXvlK5bd-6cvolAcmvrKoMLwfPu1dbRTmLihblmLF-pG_-n9blY3iZQ&s=PYtGPY4zEe6GWZtwZHpV27q6S2pbYp7pt4MPLpyrKFg&e= 
> I-love.SAKURA.ne.jp/T/
> Reported-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
> Closes: https://urldefense.proofpoint.com/v2/url?u=https-3A__syzkaller.appspot.com_bug-3Fextid-3D97e301b4b82ae803d21b&d=DwIDAg&c=BSDicqBQBDjDI9RkVyTcHQ&r=q5bIm4AXMzc8NJu1_RGmnQ2fMWKq4Y4RAkElvUgSs00&m=hbRXIRbizwXvlK5bd-6cvolAcmvrKoMLwfPu1dbRTmLihblmLF-pG_-n9blY3iZQ&s=m0spR05lFH2IzlSg01JM1INNQQJftf6373Pu8q1Bz6Q&e= 
> Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
> Tested-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
> Signed-off-by: George Anthony Vernon <contact@gvernon.com>
> ---
> Changes from V3->V4:
> - Remove explicit "do nothing" case statement labels in favor of
>   implicit default
> - Check superblock for bad inode
> 
> Changes from V2->V3:
> - Use HFS-specific references in preference to TN1150
> - Remove Tetsuo's additional superblock check from patch series
> - Rename is_valid_cnid() -> is_valid_catalog_record()
> - Add static inline hfs_inode_type() helper function
> - Do not BUG() on detected bad inode, use pr_warn()
> 
> Changes from V1->V2:
> - is_valid_cnid prototype now takes u32 and u8 types
> - Add fsck advice in dmesg
> - Replace make_bad_inode calls in hfs_read_inode with gotos
> - Reuse same check in hfs_write_inode
> - Disallow HFS_POR_CNID, HFS_BAD_CNID, and HFS_EXCH_CNID
> - Add Tetsuo's patch to mine and make it a patch series
> 
>  fs/hfs/inode.c | 65 +++++++++++++++++++++++++++++++++++++++-----------
>  fs/hfs/super.c |  2 +-
>  2 files changed, 52 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
> index 878535db64d6..469ea6401d47 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -340,6 +340,31 @@ static int hfs_test_inode(struct inode *inode, void *data)
>  	}
>  }
>  
> +/*
> + * is_valid_catalog_record
> + *
> + * Validate the CNID of a catalog record
> + */
> +static inline
> +bool is_valid_catalog_record(u32 cnid, u8 type)
> +{
> +	if (likely(cnid >= HFS_FIRSTUSER_CNID))
> +		return true;
> +
> +	switch (cnid) {
> +	case HFS_ROOT_CNID:
> +		return type == HFS_CDR_DIR;
> +	case HFS_EXT_CNID:
> +	case HFS_CAT_CNID:
> +		return type == HFS_CDR_FIL;
> +	default:
> +		/* Invalid reserved CNID */
> +		break;
> +	}
> +
> +	return false;
> +}
> +
>  /*
>   * hfs_read_inode
>   */
> @@ -369,6 +394,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
>  	rec = idata->rec;
>  	switch (rec->type) {
>  	case HFS_CDR_FIL:
> +		if (!is_valid_catalog_record(rec->file.FlNum, rec->type))

The rec->type is s8 data type [1]. So, it's OK to use it as it is. However, rec-
>file.FlNum is __be32 data type [2]. So, it needs to use be32_to_cpu() here.

> +			goto make_bad_inode;
>  		if (!HFS_IS_RSRC(inode)) {
>  			hfs_inode_read_fork(inode, rec->file.ExtRec, rec->file.LgLen,
>  					    rec->file.PyLen, be16_to_cpu(rec->file.ClpSize));
> @@ -390,6 +417,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
>  		inode->i_mapping->a_ops = &hfs_aops;
>  		break;
>  	case HFS_CDR_DIR:
> +		if (!is_valid_catalog_record(rec->dir.DirID, rec->type))

Ditto.

> +			goto make_bad_inode;
>  		inode->i_ino = be32_to_cpu(rec->dir.DirID);
>  		inode->i_size = be16_to_cpu(rec->dir.Val) + 2;
>  		HFS_I(inode)->fs_blocks = 0;
> @@ -399,8 +428,13 @@ static int hfs_read_inode(struct inode *inode, void *data)
>  		inode->i_op = &hfs_dir_inode_operations;
>  		inode->i_fop = &hfs_dir_operations;
>  		break;
> +	make_bad_inode:
> +		pr_warn("Invalid inode with cnid %lu\n", inode->i_ino);

I assume that inode->i_ino could be not set up here. Am I right? You need to use
rec->file.FlNum or rec->dir.DirID here.

> +		pr_warn("Volume is probably corrupted, try performing fsck.\n");
> +		fallthrough;
>  	default:
>  		make_bad_inode(inode);
> +		break;
>  	}
>  	return 0;
>  }
> @@ -448,6 +482,11 @@ void hfs_inode_write_fork(struct inode *inode, struct hfs_extent *ext,
>  					 HFS_SB(inode->i_sb)->alloc_blksz);
>  }
>  
> +static inline u8 hfs_inode_type(struct inode *inode)
> +{
> +	return S_ISDIR(inode->i_mode) ? HFS_CDR_DIR : HFS_CDR_FIL;
> +}
> +
>  int hfs_write_inode(struct inode *inode, struct writeback_control *wbc)
>  {
>  	struct inode *main_inode = inode;
> @@ -460,20 +499,18 @@ int hfs_write_inode(struct inode *inode, struct writeback_control *wbc)
>  	if (res)
>  		return res;
>  
> -	if (inode->i_ino < HFS_FIRSTUSER_CNID) {
> -		switch (inode->i_ino) {
> -		case HFS_ROOT_CNID:
> -			break;
> -		case HFS_EXT_CNID:
> -			hfs_btree_write(HFS_SB(inode->i_sb)->ext_tree);
> -			return 0;
> -		case HFS_CAT_CNID:
> -			hfs_btree_write(HFS_SB(inode->i_sb)->cat_tree);
> -			return 0;
> -		default:
> -			BUG();
> -			return -EIO;
> -		}
> +	if (!is_valid_catalog_record(inode->i_ino, hfs_inode_type(inode)))
> +		return -EIO;
> +
> +	switch (inode->i_ino) {
> +	case HFS_EXT_CNID:
> +		hfs_btree_write(HFS_SB(inode->i_sb)->ext_tree);
> +		return 0;
> +	case HFS_CAT_CNID:
> +		hfs_btree_write(HFS_SB(inode->i_sb)->cat_tree);
> +		return 0;
> +	default:
> +		break;

I am still worried that we do nothing here. I prefer to return some error if
is_valid_catalog_record() failed somehow.

Thanks,
Slava.

>  	}
>  
>  	if (HFS_IS_RSRC(inode))
> diff --git a/fs/hfs/super.c b/fs/hfs/super.c
> index a4f2a2bfa6d3..060421770147 100644
> --- a/fs/hfs/super.c
> +++ b/fs/hfs/super.c
> @@ -369,7 +369,7 @@ static int hfs_fill_super(struct super_block *sb, struct fs_context *fc)
>  	res = -EINVAL;
>  	root_inode = hfs_iget(sb, &fd.search_key->cat, &rec);
>  	hfs_find_exit(&fd);
> -	if (!root_inode)
> +	if (!root_inode || is_bad_inode(root_inode))
>  		goto bail_no_root;
>  
>  	set_default_d_op(sb, &hfs_dentry_operations);

[1]
https://elixir.bootlin.com/linux/v6.19/source/include/linux/hfs_common.h#L443
[2]
https://elixir.bootlin.com/linux/v6.19/source/include/linux/hfs_common.h#L397