[PATCH] hfs: Validate CNIDs in hfs_read_inode

[PATCH] 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 according to Apple technical note TN1150.

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
Tested-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
Signed-off-by: George Anthony Vernon <contact@gvernon.com>
---
 fs/hfs/inode.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index 9cd449913dc8..6f893011492a 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -321,6 +321,30 @@ static int hfs_test_inode(struct inode *inode, void *data)
 	}
 }
 
+/*
+ * is_valid_cnid
+ *
+ * Validate the CNID of a catalog record read from disk
+ */
+static bool is_valid_cnid(unsigned long cnid, s8 type)
+{
+	if (likely(cnid >= HFS_FIRSTUSER_CNID))
+		return true;
+
+	switch (cnid) {
+	case HFS_POR_CNID:
+	case HFS_ROOT_CNID:
+		return type == HFS_CDR_DIR;
+	case HFS_EXT_CNID:
+	case HFS_CAT_CNID:
+	case HFS_BAD_CNID:
+	case HFS_EXCH_CNID:
+		return type == HFS_CDR_FIL;
+	default:
+		return false;
+	}
+}
+
 /*
  * hfs_read_inode
  */
@@ -359,6 +383,11 @@ static int hfs_read_inode(struct inode *inode, void *data)
 		}
 
 		inode->i_ino = be32_to_cpu(rec->file.FlNum);
+		if (!is_valid_cnid(inode->i_ino, HFS_CDR_FIL)) {
+			pr_warn("rejected cnid %lu\n", inode->i_ino);
+			make_bad_inode(inode);
+			break;
+		}
 		inode->i_mode = S_IRUGO | S_IXUGO;
 		if (!(rec->file.Flags & HFS_FIL_LOCK))
 			inode->i_mode |= S_IWUGO;
@@ -372,6 +401,11 @@ static int hfs_read_inode(struct inode *inode, void *data)
 		break;
 	case HFS_CDR_DIR:
 		inode->i_ino = be32_to_cpu(rec->dir.DirID);
+		if (!is_valid_cnid(inode->i_ino, HFS_CDR_DIR)) {
+			pr_warn("rejected cnid %lu\n", inode->i_ino);
+			make_bad_inode(inode);
+			break;
+		}
 		inode->i_size = be16_to_cpu(rec->dir.Val) + 2;
 		HFS_I(inode)->fs_blocks = 0;
 		inode->i_mode = S_IFDIR | (S_IRWXUGO & ~hsb->s_dir_umask);
-- 
2.50.1

Re: [PATCH] hfs: Validate CNIDs in hfs_read_inode

[Viacheslav Dubeyko]

On Fri, 2025-10-03 at 03:45 +0100, 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 according to Apple technical note TN1150.
> 
> 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  
> Tested-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com

Let's pay respect to previous efforts. I am suggesting to add this line:

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Are you OK with it?

> Signed-off-by: George Anthony Vernon <contact@gvernon.com>
> ---
>  fs/hfs/inode.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
> index 9cd449913dc8..6f893011492a 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -321,6 +321,30 @@ static int hfs_test_inode(struct inode *inode, void *data)
>  	}
>  }
>  
> +/*
> + * is_valid_cnid
> + *
> + * Validate the CNID of a catalog record read from disk
> + */
> +static bool is_valid_cnid(unsigned long cnid, s8 type)

I think we can declare like this:

static inline
bool is_valid_cnid(unsigned long cnid, s8 type)

Why cnid has unsigned long type? The u32 is pretty enough.

Why type has signed type (s8)? We don't expect negative values here. Let's use
u8 type.

> +{
> +	if (likely(cnid >= HFS_FIRSTUSER_CNID))
> +		return true;
> +
> +	switch (cnid) {
> +	case HFS_POR_CNID:
> +	case HFS_ROOT_CNID:
> +		return type == HFS_CDR_DIR;
> +	case HFS_EXT_CNID:
> +	case HFS_CAT_CNID:
> +	case HFS_BAD_CNID:
> +	case HFS_EXCH_CNID:
> +		return type == HFS_CDR_FIL;
> +	default:
> +		return false;

We can simply have default that is doing nothing:

default:
    /* continue logic */
    break;

> +	}

I believe that it will be better to return false by default here (after switch).

> +}
> +
>  /*
>   * hfs_read_inode
>   */
> @@ -359,6 +383,11 @@ static int hfs_read_inode(struct inode *inode, void *data)
>  		}
>  
>  		inode->i_ino = be32_to_cpu(rec->file.FlNum);
> +		if (!is_valid_cnid(inode->i_ino, HFS_CDR_FIL)) {
> +			pr_warn("rejected cnid %lu\n", inode->i_ino);

The syzbot reported the issue on specially corrupted volume. So, probably, it
will be better to mentioned that "volume is probably corrupted" and to advice to
run FSCK tool.

> +			make_bad_inode(inode);

We already have make_bad_inode(inode) under default case of switch. Let's jump
there without replicating this call multiple times. It makes the code more
complicated.

> +			break;
> +		}
>  		inode->i_mode = S_IRUGO | S_IXUGO;
>  		if (!(rec->file.Flags & HFS_FIL_LOCK))
>  			inode->i_mode |= S_IWUGO;
> @@ -372,6 +401,11 @@ static int hfs_read_inode(struct inode *inode, void *data)
>  		break;
>  	case HFS_CDR_DIR:
>  		inode->i_ino = be32_to_cpu(rec->dir.DirID);
> +		if (!is_valid_cnid(inode->i_ino, HFS_CDR_DIR)) {
> +			pr_warn("rejected cnid %lu\n", inode->i_ino);

Ditto.

> +			make_bad_inode(inode);

Ditto.

> +			break;
> +		}
>  		inode->i_size = be16_to_cpu(rec->dir.Val) + 2;
>  		HFS_I(inode)->fs_blocks = 0;
>  		inode->i_mode = S_IFDIR | (S_IRWXUGO & ~hsb->s_dir_umask);

We have practically the same check for the case of hfs_write_inode():

int hfs_write_inode(struct inode *inode, struct writeback_control *wbc)
{
	struct inode *main_inode = inode;
	struct hfs_find_data fd;
	hfs_cat_rec rec;
	int res;

	hfs_dbg("ino %lu\n", inode->i_ino);
	res = hfs_ext_write_extent(inode);
	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;

I think we need to select something one here. :) I believe we need to remove
BUG() and return -EIO, finally. What do you think? 

		}
	}

<skipped>
}

What's about to use your check here too?

Mostly, I like your approach but the patch needs some polishing yet. ;)

Thanks,
Slava.

Re: [PATCH] hfs: Validate CNIDs in hfs_read_inode

[George Anthony Vernon]

On Fri, Oct 03, 2025 at 10:40:16PM +0000, Viacheslav Dubeyko wrote:
> Let's pay respect to previous efforts. I am suggesting to add this line:
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> 
> Are you OK with it?
I agree with paying respect to Tetsuo. The kernel docs indicate that the SoB tag
isn't used like that. Would the Suggested-by: tag be more appropriate?

> I think we can declare like this:
> 
> static inline
> bool is_valid_cnid(unsigned long cnid, s8 type)
> 
> Why cnid has unsigned long type? The u32 is pretty enough.
Because struct inode's inode number is an unsigned long.
> 
> Why type has signed type (s8)? We don't expect negative values here. Let's use
> u8 type.
Because the type field of struct hfs_cat_rec is an s8. Is there anything to gain
by casting the s8 to a u8?

> 
> > +{
> > +	if (likely(cnid >= HFS_FIRSTUSER_CNID))
> > +		return true;
> > +
> > +	switch (cnid) {
> > +	case HFS_POR_CNID:
> > +	case HFS_ROOT_CNID:
> > +		return type == HFS_CDR_DIR;
> > +	case HFS_EXT_CNID:
> > +	case HFS_CAT_CNID:
> > +	case HFS_BAD_CNID:
> > +	case HFS_EXCH_CNID:
> > +		return type == HFS_CDR_FIL;
> > +	default:
> > +		return false;
> 
> We can simply have default that is doing nothing:
> 
> default:
>     /* continue logic */
>     break;
> 
> > +	}
> 
> I believe that it will be better to return false by default here (after switch).
We can do that, but why would it be better, is it an optimisation? We don't have
any logic to continue.

> > +			break;
> > +		}
> >  		inode->i_size = be16_to_cpu(rec->dir.Val) + 2;
> >  		HFS_I(inode)->fs_blocks = 0;
> >  		inode->i_mode = S_IFDIR | (S_IRWXUGO & ~hsb->s_dir_umask);
> 
> We have practically the same check for the case of hfs_write_inode():
> 
> int hfs_write_inode(struct inode *inode, struct writeback_control *wbc)
> {
> 	struct inode *main_inode = inode;
> 	struct hfs_find_data fd;
> 	hfs_cat_rec rec;
> 	int res;
> 
> 	hfs_dbg("ino %lu\n", inode->i_ino);
> 	res = hfs_ext_write_extent(inode);
> 	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;
> 
> I think we need to select something one here. :) I believe we need to remove
> BUG() and return -EIO, finally. What do you think? 

I think that with validation of inodes in hfs_read_inode this code path should
no longer be reachable by poking the kernel interface from userspace. If it is
ever reached, it means kernel logic is broken, so it should be treated as a bug.

> 
> 		}
> 	}
> 
> <skipped>
> }
> 
> What's about to use your check here too?

Let's do that, I'll include it in V2.

> 
> Mostly, I like your approach but the patch needs some polishing yet. ;)
> 
> Thanks,
> Slava.

Thank you for taking the time to give detailed feedback, I really appreciate it.

George

Re: [PATCH] hfs: Validate CNIDs in hfs_read_inode

[Viacheslav Dubeyko]

On Sat, 2025-10-04 at 02:25 +0100, George Anthony Vernon wrote:
> On Fri, Oct 03, 2025 at 10:40:16PM +0000, Viacheslav Dubeyko wrote:
> > Let's pay respect to previous efforts. I am suggesting to add this
> > line:
> > 
> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > 
> > Are you OK with it?
> I agree with paying respect to Tetsuo. The kernel docs indicate that
> the SoB tag
> isn't used like that. Would the Suggested-by: tag be more
> appropriate?
> 

Frankly speaking, I don't see how Suggested-by is applicable here. :)
My point was that if you mentioned the previous discussion, then it
means that you read it. And it sounds to me that your patch is
following to the points are discussed there. So, your code is
inevitably based on the code is shared during that discussion. This is
why I suggested the Signed-off-by. But if you think that it's not
correct logic for you, then I am completely OK. :)

> > I think we can declare like this:
> > 
> > static inline
> > bool is_valid_cnid(unsigned long cnid, s8 type)
> > 
> > Why cnid has unsigned long type? The u32 is pretty enough.
> Because struct inode's inode number is an unsigned long.

The Catalog Node ID (CNID) is identification number of item in Catalog
File of HFS/HFS+ file system. And it hasn't direct relation with inode
number. The Technical Note TN1150 [1] define it as:

The catalog node ID is defined by the CatalogNodeID data type.

typedef UInt32 HFSCatalogNodeID;

The hfs.h declares CNID as __be32 always. Also, hfsplus_raw.h defines
CNID as: typedef __be32 hfsplus_cnid;.

So, it cannot be bigger than 32 bits. But unsigned long could be bigger
than unsigned int. Potentially, unsigned long could be 64 bits on some
platforms.

> > 
> > Why type has signed type (s8)? We don't expect negative values
> > here. Let's use
> > u8 type.
> Because the type field of struct hfs_cat_rec is an s8. Is there
> anything to gain
> by casting the s8 to a u8?
> 

I am not completely sure that s8 was correct declaration type in struct
hfs_cat_rec and other ones. But if we will use s8 as input parameter,
then we could have soon another syzbot report about crash because this
framework has generated negative values as input parameter. And I would
like to avoid such situation by using u8 data type. Especially,
because, negative values don't make sense for type of object.

> > 
> > > +{
> > > +	if (likely(cnid >= HFS_FIRSTUSER_CNID))
> > > +		return true;
> > > +
> > > +	switch (cnid) {
> > > +	case HFS_POR_CNID:
> > > +	case HFS_ROOT_CNID:
> > > +		return type == HFS_CDR_DIR;
> > > +	case HFS_EXT_CNID:
> > > +	case HFS_CAT_CNID:
> > > +	case HFS_BAD_CNID:
> > > +	case HFS_EXCH_CNID:
> > > +		return type == HFS_CDR_FIL;
> > > +	default:
> > > +		return false;
> > 
> > We can simply have default that is doing nothing:
> > 
> > default:
> >     /* continue logic */
> >     break;
> > 
> > > +	}
> > 
> > I believe that it will be better to return false by default here
> > (after switch).
> We can do that, but why would it be better, is it an optimisation? We
> don't have
> any logic to continue.

We have this function flow:

bool is_valid_cnid()
{
   if (condition)
      return <something>;

   switch () {
   case 1:
      return something;
   }
}

Some compilers can treat this like function should return value but has
no return by default. And it could generate warnings. So, this is why I
suggested to have return at the end of function by default.

> 
> > > +			break;
> > > +		}
> > >  		inode->i_size = be16_to_cpu(rec->dir.Val) + 2;
> > >  		HFS_I(inode)->fs_blocks = 0;
> > >  		inode->i_mode = S_IFDIR | (S_IRWXUGO & ~hsb-
> > > >s_dir_umask);
> > 
> > We have practically the same check for the case of
> > hfs_write_inode():
> > 
> > int hfs_write_inode(struct inode *inode, struct writeback_control
> > *wbc)
> > {
> > 	struct inode *main_inode = inode;
> > 	struct hfs_find_data fd;
> > 	hfs_cat_rec rec;
> > 	int res;
> > 
> > 	hfs_dbg("ino %lu\n", inode->i_ino);
> > 	res = hfs_ext_write_extent(inode);
> > 	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;
> > 
> > I think we need to select something one here. :) I believe we need
> > to remove
> > BUG() and return -EIO, finally. What do you think? 
> 
> I think that with validation of inodes in hfs_read_inode this code
> path should
> no longer be reachable by poking the kernel interface from userspace.
> If it is
> ever reached, it means kernel logic is broken, so it should be
> treated as a bug.
> 

We already have multiple syzbot reports with kernel crashes for
likewise BUG() statements in HFS/HFS+ code. From one point of view, it
is better to return error instead of crashing kernel. From another
point of view, the 'return -EIO' is never called because we have BUG()
before. So, these two statements together don't make sense. This is why
I am suggesting to rework this code.

Thanks,
Slava.

> > 
> > 		}
> > 	}
> > 
> > <skipped>
> > }
> > 
> > What's about to use your check here too?
> 
> Let's do that, I'll include it in V2.
> 
> > 
> > Mostly, I like your approach but the patch needs some polishing
> > yet. ;)
> > 
> > Thanks,
> > Slava.
> 
> Thank you for taking the time to give detailed feedback, I really
> appreciate it.
> 
> George

[1]
https://dubeyko.com/development/FileSystems/HFSPLUS/tn1150.html#CatalogFile

Re: [PATCH] hfs: Validate CNIDs in hfs_read_inode

[Tetsuo Handa]

I found this patch. Please CC: me when posting V2.

On 2025/10/07 22:40, Viacheslav Dubeyko wrote:
> On Sat, 2025-10-04 at 02:25 +0100, George Anthony Vernon wrote:
>> On Fri, Oct 03, 2025 at 10:40:16PM +0000, Viacheslav Dubeyko wrote:
>>> Let's pay respect to previous efforts. I am suggesting to add this
>>> line:
>>>
>>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>>>
>>> Are you OK with it?
>> I agree with paying respect to Tetsuo. The kernel docs indicate that
>> the SoB tag
>> isn't used like that. Would the Suggested-by: tag be more
>> appropriate?
>>

I'm not suggesting this change. Therefore, Cc: might match.

But I don't like

  Tested-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com

line, for syzbot only tested one cnid which was embedded in the
reproducer. My modified reproducer which tests all range still hits
BUG() when the inode number of the record retrieved as a result of
hfs_cat_find_brec(HFS_ROOT_CNID) is HFS_POR_CNID. That is why I push
https://lkml.kernel.org/r/427fcb57-8424-4e52-9f21-7041b2c4ae5b@I-love.SAKURA.ne.jp
as a fix for this problem (and you can propose this patch as a
further sanity check). Unless

>>>
>>>> +{
>>>> +	if (likely(cnid >= HFS_FIRSTUSER_CNID))
>>>> +		return true;
>>>> +
>>>> +	switch (cnid) {
>>>> +	case HFS_POR_CNID:

we disable HFS_POR_CNID case (which I guess it is wrong to do so),
we shall hit BUG() in hfs_write_inode().

>>>> +	case HFS_ROOT_CNID:
>>>> +		return type == HFS_CDR_DIR;
>>>> +	case HFS_EXT_CNID:
>>>> +	case HFS_CAT_CNID:
>>>> +	case HFS_BAD_CNID:
>>>> +	case HFS_EXCH_CNID:
>>>> +		return type == HFS_CDR_FIL;
>>>> +	default:
>>>> +		return false;
>>>



>>> int hfs_write_inode(struct inode *inode, struct writeback_control
>>> *wbc)
>>> {
>>> 	struct inode *main_inode = inode;
>>> 	struct hfs_find_data fd;
>>> 	hfs_cat_rec rec;
>>> 	int res;
>>>
>>> 	hfs_dbg("ino %lu\n", inode->i_ino);
>>> 	res = hfs_ext_write_extent(inode);
>>> 	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;
>>>
>>> I think we need to select something one here. :) I believe we need
>>> to remove
>>> BUG() and return -EIO, finally. What do you think? 

I think that removing this BUG() now is wrong.
Without my patch, the inode number of the record retrieved as a
result of hfs_cat_find_brec(HFS_ROOT_CNID) can be HFS_POR_CNID or
greater than HFS_FIRSTUSER_CNID, which I think is a logical error
in the filesystem image.

>>
>> I think that with validation of inodes in hfs_read_inode this code
>> path should
>> no longer be reachable by poking the kernel interface from userspace.
>> If it is
>> ever reached, it means kernel logic is broken, so it should be
>> treated as a bug.

Your patch is incomplete. Please also apply my patch.

Re: [PATCH] hfs: Validate CNIDs in hfs_read_inode

[George Anthony Vernon]

Hi Tetsuo,

On Thu, Oct 09, 2025 at 09:57:33PM +0900, Tetsuo Handa wrote:
> I found this patch. Please CC: me when posting V2.

Sorry I forgot to CC you last time :)

> I'm not suggesting this change. Therefore, Cc: might match.

Sure, I have added a CC tag for you in V2 which I'm currently testing.

> further sanity check). Unless
> 
> >>>
> >>>> +{
> >>>> +	if (likely(cnid >= HFS_FIRSTUSER_CNID))
> >>>> +		return true;
> >>>> +
> >>>> +	switch (cnid) {
> >>>> +	case HFS_POR_CNID:
> 
> we disable HFS_POR_CNID case (which I guess it is wrong to do so),
> we shall hit BUG() in hfs_write_inode().
> 
> >>>> +	case HFS_ROOT_CNID:
> >>>> +		return type == HFS_CDR_DIR;
> >>>> +	case HFS_EXT_CNID:
> >>>> +	case HFS_CAT_CNID:
> >>>> +	case HFS_BAD_CNID:
> >>>> +	case HFS_EXCH_CNID:
> >>>> +		return type == HFS_CDR_FIL;
> >>>> +	default:
> >>>> +		return false;
> >>>
>
> I think that removing this BUG() now is wrong.

I think HFS_POR_CNID case should be disallowed. There is no real
underlying file with that CNID. If we ever found a record with that CNID
it would mean the filesystem image was broken, and if we ever try to
write a record with that CNID, it means we screwed up.

> Without my patch, the inode number of the record retrieved as a
> result of hfs_cat_find_brec(HFS_ROOT_CNID) can be HFS_POR_CNID or
> greater than HFS_FIRSTUSER_CNID, which I think is a logical error
> in the filesystem image.
> 
> Your patch is incomplete. Please also apply my patch.
> 
I agree your check is good to catch root inode's i_ino > 15 (is this
reachable?) and I'd like to add it. Would you be happy if I make a
2-part patch series with your patch second, keeping your sign-off on it?

Thanks,

George

Re: [PATCH] hfs: Validate CNIDs in hfs_read_inode

[Tetsuo Handa]

On 2025/10/29 12:20, George Anthony Vernon wrote:
> I think HFS_POR_CNID case should be disallowed. There is no real
> underlying file with that CNID. If we ever found a record with that CNID
> it would mean the filesystem image was broken, and if we ever try to
> write a record with that CNID, it means we screwed up.

Hmm, your interpretation does not match what Viacheslav Dubeyko interpreted

  hfs_read_inode() can be called for the root directory and parent of
  the root cases. So, HFS_POR_CNID and HFS_ROOT_CNID are legitimate values.

at https://lkml.kernel.org/r/9a18338da59460bd5c95605d8b10f895a0b7dbb8.camel@ibm.com .

But if HFS_POR_CNID is not allowed, you can inline is_valid_cnid() for HFS_CDR_DIR case
like https://lkml.kernel.org/r/23498435-ee11-4eb9-9be9-8460a6fa17f1@I-love.SAKURA.ne.jp .

> I agree your check is good to catch root inode's i_ino > 15 (is this
> reachable?) and I'd like to add it. Would you be happy if I make a
> 2-part patch series with your patch second, keeping your sign-off on it?

OK.

[PATCH v2 0/2] hfs: Validate CNIDs read from filesystem

[George Anthony Vernon]

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

This patch series contains two patches which work together to prevent
bad catalog records from being read. Previously it was possible for a
malformed HFS filesystem image to reach a BUG() in writeback. I propose
to fix this by verifying that CNIDs are allowed ones when constructing
an inode in hfs_read_inode(). Tetsuo Handa's additional check in
hfs_fill_super makes sure the root inode's CNID is correct, handling an
edge case where the records' directory CNID > 15.

I think we should continue returning BUG() in hfs_write_inode() because
it is a filesystem implementation error if we ever allow an inode to be
constructed from a bad CNID. Now we properly guard our reads in
hfs_read_inode(), records with bad CNIDs should not get so far as to
initialise inodes which are queued for writeback.

I'm suggesting to disallow HFS_POR_CNID because there is no 'real'
record with that CNID or corresponding file, it doesn't make sense to
present an inode for it to the VFS when it exists only as a dummy
reference in our internal btree. Similarly I'm suggesting to disallow
HFS_BAD_CNID and HFS_EXCH_CNID because not only are they for internal
use, but we also do not implement bad blocks or exchange file
functionality in the Linux driver.

Thank you to Slava and Tetsuo for the feedback on V1.

George Anthony Vernon (2):
  hfs: Validate CNIDs in hfs_read_inode
  hfs: Update sanity check of the root record

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

-- 
2.50.1

[PATCH v2 1/2] 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 according to Apple technical note TN1150.

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>
---
 fs/hfs/inode.c | 67 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 53 insertions(+), 14 deletions(-)

diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index 9cd449913dc8..bc346693941d 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -321,6 +321,38 @@ static int hfs_test_inode(struct inode *inode, void *data)
 	}
 }
 
+/*
+ * is_valid_cnid
+ *
+ * Validate the CNID of a catalog record
+ */
+static inline
+bool 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;
+	case HFS_POR_CNID:
+		/* No valid record with this CNID */
+		break;
+	case HFS_BAD_CNID:
+	case HFS_EXCH_CNID:
+		/* Not implemented */
+		break;
+	default:
+		/* Invalid reserved CNID */
+		break;
+	}
+
+	return false;
+}
+
 /*
  * hfs_read_inode
  */
@@ -350,6 +382,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
 	rec = idata->rec;
 	switch (rec->type) {
 	case HFS_CDR_FIL:
+		if (!is_valid_cnid(rec->file.FlNum, HFS_CDR_FIL))
+			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));
@@ -371,6 +405,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_cnid(rec->dir.DirID, HFS_CDR_DIR))
+			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;
@@ -380,8 +416,12 @@ 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("rejected cnid %lu. Volume is probably corrupted, try performing fsck.\n", inode->i_ino);
+		fallthrough;
 	default:
 		make_bad_inode(inode);
+		break;
 	}
 	return 0;
 }
@@ -441,20 +481,19 @@ 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_cnid(inode->i_ino,
+			   S_ISDIR(inode->i_mode) ? HFS_CDR_DIR : HFS_CDR_FIL))
+		BUG();
+
+	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))
-- 
2.50.1

Re: [PATCH v2 1/2] hfs: Validate CNIDs in hfs_read_inode

[Viacheslav Dubeyko]

On Tue, 2025-11-04 at 01:47 +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 according to Apple technical note TN1150.

The TN1150 technical note describes HFS+ file system and it needs to take into
account the difference between HFS and HFS+. So, it is not completely correct
for the case of HFS to follow to the TN1150 technical note as it is.

> 
> 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>
> ---
>  fs/hfs/inode.c | 67 +++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 53 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
> index 9cd449913dc8..bc346693941d 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -321,6 +321,38 @@ static int hfs_test_inode(struct inode *inode, void *data)
>  	}
>  }
>  
> +/*
> + * is_valid_cnid
> + *
> + * Validate the CNID of a catalog record
> + */
> +static inline
> +bool 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;
> +	case HFS_POR_CNID:
> +		/* No valid record with this CNID */
> +		break;
> +	case HFS_BAD_CNID:

HFS is ancient file system that was needed to work with floppy disks. And bad
sectors management was regular task and responsibility of HFS for the case of
floppy disks (HDD was also not very reliable at that times). So, HFS implements
the bad block management. It means that, potentially, Linux kernel could need to
mount a file system volume that created by ancient Mac OS.

I don't think that it's correct management of HFS_BAD_CNID. We must to expect to
have such CNID for the case of HFS.

> +	case HFS_EXCH_CNID:
> +		/* Not implemented */
> +		break;
> +	default:
> +		/* Invalid reserved CNID */
> +		break;
> +	}
> +
> +	return false;
> +}
> +
>  /*
>   * hfs_read_inode
>   */
> @@ -350,6 +382,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
>  	rec = idata->rec;
>  	switch (rec->type) {
>  	case HFS_CDR_FIL:
> +		if (!is_valid_cnid(rec->file.FlNum, HFS_CDR_FIL))
> +			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));
> @@ -371,6 +405,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_cnid(rec->dir.DirID, HFS_CDR_DIR))
> +			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;
> @@ -380,8 +416,12 @@ 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("rejected cnid %lu. Volume is probably corrupted, try performing fsck.\n", inode->i_ino);

The "invalid cnid" could sound more relevant than "rejected cnid" for my taste.

The whole message is too long. What's about to have two messages here?

pr_warn("invalid 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;
>  }
> @@ -441,20 +481,19 @@ 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_cnid(inode->i_ino,
> +			   S_ISDIR(inode->i_mode) ? HFS_CDR_DIR : HFS_CDR_FIL))

What's about to introduce static inline function or local variable for
S_ISDIR(inode->i_mode) ? HFS_CDR_DIR : HFS_CDR_FIL? I don't like this two line
implementation.

> +		BUG();

I am completely against of leaving BUG() here. Several fixes of syzbot issues
were the exchanging BUG() on returning error code. I don't want to investigate
the another syzbot issue that will involve this BUG() here. Let's return error
code here.

Usually, it makes sense to have BUG() for debug mode and to return error code
for the case of release mode. But we don't have the debug mode for HFS code.

Thanks,
Slava.

> +
> +	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))

[PATCH v2 2/2] hfs: Update sanity check of the root record

[George Anthony Vernon]

syzbot is reporting that BUG() in hfs_write_inode() fires upon unmount
operation when the inode number of the record retrieved as a result of
hfs_cat_find_brec(HFS_ROOT_CNID) is not HFS_ROOT_CNID, for commit
b905bafdea21 ("hfs: Sanity check the root record") checked the record
size and the record type but did not check the inode number.

Reported-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: George Anthony Vernon <contact@gvernon.com>
---
 fs/hfs/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/hfs/super.c b/fs/hfs/super.c
index 47f50fa555a4..a7dd20f2d743 100644
--- a/fs/hfs/super.c
+++ b/fs/hfs/super.c
@@ -358,7 +358,7 @@ 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 || rec.dir.DirID != cpu_to_be32(HFS_ROOT_CNID))
 			res = -EIO;
 	}
 	if (res)
-- 
2.50.1

Re: [PATCH v2 2/2] hfs: Update sanity check of the root record

[Viacheslav Dubeyko]

On Tue, 2025-11-04 at 01:47 +0000, George Anthony Vernon wrote:
> syzbot is reporting that BUG() in hfs_write_inode() fires upon unmount
> operation when the inode number of the record retrieved as a result of
> hfs_cat_find_brec(HFS_ROOT_CNID) is not HFS_ROOT_CNID, for commit
> b905bafdea21 ("hfs: Sanity check the root record") checked the record
> size and the record type but did not check the inode number.
> 
> Reported-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b  
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: George Anthony Vernon <contact@gvernon.com>
> ---
>  fs/hfs/super.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/hfs/super.c b/fs/hfs/super.c
> index 47f50fa555a4..a7dd20f2d743 100644
> --- a/fs/hfs/super.c
> +++ b/fs/hfs/super.c
> @@ -358,7 +358,7 @@ 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 || rec.dir.DirID != cpu_to_be32(HFS_ROOT_CNID))

This check is completely unnecessary. Because, we have hfs_iget() then [1]:

	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;

The hfs_iget() calls iget5_locked() [2]:

struct inode *hfs_iget(struct super_block *sb, struct hfs_cat_key *key,
hfs_cat_rec *rec)
{
	struct hfs_iget_data data = { key, rec };
	struct inode *inode;
	u32 cnid;

	switch (rec->type) {
	case HFS_CDR_DIR:
		cnid = be32_to_cpu(rec->dir.DirID);
		break;
	case HFS_CDR_FIL:
		cnid = be32_to_cpu(rec->file.FlNum);
		break;
	default:
		return NULL;
	}
	inode = iget5_locked(sb, cnid, hfs_test_inode, hfs_read_inode, &data);
	if (inode && (inode->i_state & I_NEW))
		unlock_new_inode(inode);
	return inode;
}

And iget5_locked() calls hfs_read_inode(). And hfs_read_inode() will call
is_valid_cnid() after applying your patch. So, is_valid_cnid() in
hfs_read_inode() can completely manage the issue. This is why we don't need in
this modification after your first patch.

But I think we need to check that root_inode is not bad inode afterwards:

	root_inode = hfs_iget(sb, &fd.search_key->cat, &rec);
	hfs_find_exit(&fd);
	if (!root_inode || is_bad_inode(root_inode))
		goto bail_no_root;

Thanks,
Slava.

>  			res = -EIO;
>  	}
>  	if (res)

[1] https://elixir.bootlin.com/linux/v6.18-rc4/source/fs/hfs/super.c#L367
[2] https://elixir.bootlin.com/linux/v6.18-rc4/source/fs/hfs/inode.c#L414