[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

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

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

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

[George Anthony Vernon]

On Tue, Nov 04, 2025 at 11:01:31PM +0000, Viacheslav Dubeyko wrote:
> 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]:
> 
> The hfs_iget() calls iget5_locked() [2]:
> 
> 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.
> 

I think Tetsuo's concern is that a directory catalog record with
cnid > 15 might be returned as a result of hfs_bnode_read, which
is_valid_cnid() would not protect against. I've satisfied myself that
hfs_bnode_read() in hfs_fill_super() will populate hfs_find_data fd
correctly and crash out if it failed to find a record with root CNID so
this path is unreachable and there is no need for the second 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;

Agreed, I see hfs_read_inode might return a bad inode. Thanks for
catching this. I noticed also that it returns an int but the return
value holds no meaning; it is always zero.

> Thanks,
> Slava.
>

Many thanks again,

George

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

[Viacheslav Dubeyko]

On Mon, 2025-11-10 at 23:03 +0000, George Anthony Vernon wrote:
> On Tue, Nov 04, 2025 at 11:01:31PM +0000, Viacheslav Dubeyko wrote:
> > 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]:
> > 
> > The hfs_iget() calls iget5_locked() [2]:
> > 
> > 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.
> > 
> 
> I think Tetsuo's concern is that a directory catalog record with
> cnid > 15 might be returned as a result of hfs_bnode_read, which
> is_valid_cnid() would not protect against. I've satisfied myself that
> hfs_bnode_read() in hfs_fill_super() will populate hfs_find_data fd
> correctly and crash out if it failed to find a record with root CNID so
> this path is unreachable and there is no need for the second patch.
> 

Technically speaking, we can adopt this check to be completely sure that nothing
will be wrong during the mount operation. But I believe that is_valid_cnid()
should be good enough to manage this. Potential argument could be that the check
of rec.dir.DirID could be faster operation than to call hfs_iget(). But mount is
rare and not very fast operation, anyway. And if we fail to mount, then the
speed of mount operation is not very important.

> > 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;
> 
> Agreed, I see hfs_read_inode might return a bad inode. Thanks for
> catching this. I noticed also that it returns an int but the return
> value holds no meaning; it is always zero.
> 
> 

I've realized that hfs_write_inode() doesn't check that inode is bad like other
file systems do. Probably, we need to have this check too.

Thanks,
Slava.

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

[George Anthony Vernon]

On Tue, Nov 04, 2025 at 10:34:15PM +0000, Viacheslav Dubeyko wrote:
> 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.

I've checked Inside Macintosh: Files Chapter 2 page 70 to make sure HFS
is the same (CNIDs 1 - 5 are assigned, and all of 1-15 are reserved).
I will add this to the commit message for V3.

> > 
> > 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.
> 

HFS_BAD_CNID is reserved for internal use of the filesystem
implementation. Since we never intend to use it, there is no correct
logical path that we should ever construct an inode with CNID 5. It does
not correspond to a record that the user can open, as it is a special
CNID used only for extent records used to mark blocks as allocated so
they are not used, a behaviour which we do not implement in the Linux
HFS or HFS+ drivers. Disallowing this CNID will not prevent correctly
formed filesystems from being mounted. I also don't think that
presenting an internal record-keeping structure to the VFS would make
sense or would be consistent with other filesystems.

> > +	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");
> 
Good improvement!
> 
> > +		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.

Okay, I will rewrite this.

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

I prefer BUG() because I think it is a serious bug that we should not
allow for a bad inode to be written. I am willing to take responsibility
for investigating further issues if they appear as a result of this. Of
course, the final say on BUG() or -EIO is yours as the maintainer.

> 
> Thanks,
> Slava.
> 
Many thanks for your time and efforts,

George

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

[George Anthony Vernon]

On Mon, Nov 10, 2025 at 11:34:39PM +0000, Viacheslav Dubeyko wrote:
> On Mon, 2025-11-10 at 23:03 +0000, George Anthony Vernon wrote:
> > On Tue, Nov 04, 2025 at 11:01:31PM +0000, Viacheslav Dubeyko wrote:
> > > 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]:
> > > 
> > > The hfs_iget() calls iget5_locked() [2]:
> > > 
> > > 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.
> > > 
> > 
> > I think Tetsuo's concern is that a directory catalog record with
> > cnid > 15 might be returned as a result of hfs_bnode_read, which
> > is_valid_cnid() would not protect against. I've satisfied myself that
> > hfs_bnode_read() in hfs_fill_super() will populate hfs_find_data fd
> > correctly and crash out if it failed to find a record with root CNID so
> > this path is unreachable and there is no need for the second patch.
> > 
> 
> Technically speaking, we can adopt this check to be completely sure that nothing
> will be wrong during the mount operation. But I believe that is_valid_cnid()
> should be good enough to manage this. Potential argument could be that the check
> of rec.dir.DirID could be faster operation than to call hfs_iget(). But mount is
> rare and not very fast operation, anyway. And if we fail to mount, then the
> speed of mount operation is not very important.

Agreed we're not worried about speed that the mount operation can reach
fail case. The check would have value if the bnode populated in
hfs_find_data fd by hfs_cat_find_brec() is bad. That would be very
defensive, I'm not sure it's necessary.

Maybe is_valid_cnid() should be is_valid_catalog_cnid(), since that is
what it is actually testing for at the interface with the VFS. Would you
agree?

> 
> > > 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;
> > 
> > Agreed, I see hfs_read_inode might return a bad inode. Thanks for
> > catching this. I noticed also that it returns an int but the return
> > value holds no meaning; it is always zero.
> > 
> > 
> 
> I've realized that hfs_write_inode() doesn't check that inode is bad like other
> file systems do. Probably, we need to have this check too.

Good point, and similarly with HFS+. I'll take a look.

Thanks,

George

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

[Viacheslav Dubeyko]

On Tue, 2025-11-11 at 00:23 +0000, George Anthony Vernon wrote:
> On Mon, Nov 10, 2025 at 11:34:39PM +0000, Viacheslav Dubeyko wrote:
> > On Mon, 2025-11-10 at 23:03 +0000, George Anthony Vernon wrote:
> > > On Tue, Nov 04, 2025 at 11:01:31PM +0000, Viacheslav Dubeyko wrote:
> > > > 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]:
> > > > 
> > > > The hfs_iget() calls iget5_locked() [2]:
> > > > 
> > > > 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.
> > > > 
> > > 
> > > I think Tetsuo's concern is that a directory catalog record with
> > > cnid > 15 might be returned as a result of hfs_bnode_read, which
> > > is_valid_cnid() would not protect against. I've satisfied myself that
> > > hfs_bnode_read() in hfs_fill_super() will populate hfs_find_data fd
> > > correctly and crash out if it failed to find a record with root CNID so
> > > this path is unreachable and there is no need for the second patch.
> > > 
> > 
> > Technically speaking, we can adopt this check to be completely sure that nothing
> > will be wrong during the mount operation. But I believe that is_valid_cnid()
> > should be good enough to manage this. Potential argument could be that the check
> > of rec.dir.DirID could be faster operation than to call hfs_iget(). But mount is
> > rare and not very fast operation, anyway. And if we fail to mount, then the
> > speed of mount operation is not very important.
> 
> Agreed we're not worried about speed that the mount operation can reach
> fail case. The check would have value if the bnode populated in
> hfs_find_data fd by hfs_cat_find_brec() is bad. That would be very
> defensive, I'm not sure it's necessary.
> 
> Maybe is_valid_cnid() should be is_valid_catalog_cnid(), since that is
> what it is actually testing for at the interface with the VFS. Would you
> agree?
> 

CNID is abbreviation of Catalog Node ID. So, is_valid_catalog_cnid() will sound
like Catalog Catalog Node ID. :)

Thanks,
Slava.

> > 
> > > > 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;
> > > 
> > > Agreed, I see hfs_read_inode might return a bad inode. Thanks for
> > > catching this. I noticed also that it returns an int but the return
> > > value holds no meaning; it is always zero.
> > > 
> > > 
> > 
> > I've realized that hfs_write_inode() doesn't check that inode is bad like other
> > file systems do. Probably, we need to have this check too.
> 
> Good point, and similarly with HFS+. I'll take a look.
> 
> Thanks,
> 
> George

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

[Viacheslav Dubeyko]

On Tue, 2025-11-11 at 00:00 +0000, George Anthony Vernon wrote:
> On Tue, Nov 04, 2025 at 10:34:15PM +0000, Viacheslav Dubeyko wrote:
> > 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.
> 
> I've checked Inside Macintosh: Files Chapter 2 page 70 to make sure HFS
> is the same (CNIDs 1 - 5 are assigned, and all of 1-15 are reserved).
> I will add this to the commit message for V3.
> 
> > > 
> > > 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.
> > 
> 
> HFS_BAD_CNID is reserved for internal use of the filesystem
> implementation. Since we never intend to use it, there is no correct
> logical path that we should ever construct an inode with CNID 5. It does
> not correspond to a record that the user can open, as it is a special
> CNID used only for extent records used to mark blocks as allocated so
> they are not used, a behaviour which we do not implement in the Linux
> HFS or HFS+ drivers. Disallowing this CNID will not prevent correctly
> formed filesystems from being mounted. I also don't think that
> presenting an internal record-keeping structure to the VFS would make
> sense or would be consistent with other filesystems.
> 

Yes, we don't want to use it on Linux kernel side. But, potentially, the file
for bad block management could or was been created/used on Mac OS side. And if
anyone tries to mount the HFS volume with the bad block file, then we will
refuse to mount it on the Linux kernel side. This is my main worry here. But it
is not very probable situation, by the way.

> > > +	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");
> > 
> Good improvement!
> > 
> > > +		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.
> 
> Okay, I will rewrite this.
> 
> > 
> > > +		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.
> 
> I prefer BUG() because I think it is a serious bug that we should not
> allow for a bad inode to be written. I am willing to take responsibility
> for investigating further issues if they appear as a result of this. Of
> course, the final say on BUG() or -EIO is yours as the maintainer.
> 
> > 

The hfs_write_inode() will return error code and it is also bug case. But we
will not crash the kernel in such case. Why would you still like to crash the
kernel? :)

I see your point that we should not be here because we must create the bad inode
in hfs_read_inode() for the case of corrupted Catalog File's records. Let me
sleep on it. :)

Thanks,
Slava.

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

[Tetsuo Handa]

On 2025/11/11 9:23, George Anthony Vernon wrote:
>> Technically speaking, we can adopt this check to be completely sure that nothing
>> will be wrong during the mount operation. But I believe that is_valid_cnid()
>> should be good enough to manage this. Potential argument could be that the check
>> of rec.dir.DirID could be faster operation than to call hfs_iget(). But mount is
>> rare and not very fast operation, anyway. And if we fail to mount, then the
>> speed of mount operation is not very important.
> 
> Agreed we're not worried about speed that the mount operation can reach
> fail case. The check would have value if the bnode populated in
> hfs_find_data fd by hfs_cat_find_brec() is bad. That would be very
> defensive, I'm not sure it's necessary.

With my patch, mount() syscall fails with EIO unless rec.dir.DirID == 2.
Without my patch, mount() syscall succeeds and EIO is later returned when
trying to read the root directory of the mounted filesystem.

This is not a problem of speed. Fuzzing unreadable root directory is useless.
There is no point with making mount() syscall succeed.

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

[Tetsuo Handa]

On 2025/11/04 10:47, George Anthony Vernon wrote:
> +	if (!is_valid_cnid(inode->i_ino,
> +			   S_ISDIR(inode->i_mode) ? HFS_CDR_DIR : HFS_CDR_FIL))
> +		BUG();

Is it guaranteed that hfs_write_inode() and make_bad_inode() never run in parallel?
If no, this check is racy because make_bad_inode() makes S_ISDIR(inode->i_mode) == false.

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

[Viacheslav Dubeyko]

On Tue, 2025-11-11 at 23:39 +0900, Tetsuo Handa wrote:
> On 2025/11/04 10:47, George Anthony Vernon wrote:
> > +	if (!is_valid_cnid(inode->i_ino,
> > +			   S_ISDIR(inode->i_mode) ? HFS_CDR_DIR : HFS_CDR_FIL))
> > +		BUG();
> 
> Is it guaranteed that hfs_write_inode() and make_bad_inode() never run in parallel?
> If no, this check is racy because make_bad_inode() makes S_ISDIR(inode->i_mode) == false.
>  

Any inode should be completely created before any hfs_write_inode() call can
happen. So, I don't see how hfs_write_inode() and make_bad_inode() could run in
parallel.

But, maybe, I am not completely right that we need to call is_bad_inode() in
hfs_write_inode() of checking that it's bad inode. For example, ubifs is doing
it in the ubifs_write_inode() [1]. NILFS2 is doing it in nilfs_dirty_inode()
[2]. And majority of file systems call is_bad_inode() in evict_inode() methods.

Thanks,
Slava.

[1] https://elixir.bootlin.com/linux/v6.18-rc5/source/fs/ubifs/super.c#L299
[2] https://elixir.bootlin.com/linux/v6.18-rc5/source/fs/nilfs2/inode.c#L1087

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

[Viacheslav Dubeyko]

On Tue, 2025-11-11 at 23:26 +0900, Tetsuo Handa wrote:
> On 2025/11/11 9:23, George Anthony Vernon wrote:
> > > Technically speaking, we can adopt this check to be completely sure that nothing
> > > will be wrong during the mount operation. But I believe that is_valid_cnid()
> > > should be good enough to manage this. Potential argument could be that the check
> > > of rec.dir.DirID could be faster operation than to call hfs_iget(). But mount is
> > > rare and not very fast operation, anyway. And if we fail to mount, then the
> > > speed of mount operation is not very important.
> > 
> > Agreed we're not worried about speed that the mount operation can reach
> > fail case. The check would have value if the bnode populated in
> > hfs_find_data fd by hfs_cat_find_brec() is bad. That would be very
> > defensive, I'm not sure it's necessary.
> 
> With my patch, mount() syscall fails with EIO unless rec.dir.DirID == 2.
> Without my patch, mount() syscall succeeds and EIO is later returned when
> trying to read the root directory of the mounted filesystem.
> 

The file system is mounted only if hfs_fill_super() created root node and return
0 [1]. However, if hfs_iget() return bad inode [2] and we will call
is_bad_inode() here [3]:

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

then, mount will fail. So, no successful mount will happen because
is_valid_cnid() will manage the check in hfs_read_inode().

Thanks,
Slava.

> This is not a problem of speed. Fuzzing unreadable root directory is useless.
> There is no point with making mount() syscall succeed.


[1] https://elixir.bootlin.com/linux/v6.18-rc5/source/fs/hfs/super.c#L379
[2] https://elixir.bootlin.com/linux/v6.18-rc5/source/fs/hfs/super.c#L367
[3] https://elixir.bootlin.com/linux/v6.18-rc5/source/fs/hfs/super.c#L369

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

[Tetsuo Handa]

On 2025/11/12 7:56, Viacheslav Dubeyko wrote:
> The file system is mounted only if hfs_fill_super() created root node and return
> 0 [1]. However, if hfs_iget() return bad inode [2] and we will call
> is_bad_inode() here [3]:
> 
> 	root_inode = hfs_iget(sb, &fd.search_key->cat, &rec);
> 	hfs_find_exit(&fd);
> 	if (!root_inode || is_bad_inode(root_inode)) <-- call will be here
> 		goto bail_no_root;
> 
> then, mount will fail. So, no successful mount will happen because
> is_valid_cnid() will manage the check in hfs_read_inode().

Do you admit that mounting (and optionally fuzzing on) a bad inode (an inode
which was subjected to make_bad_inode()) is useless?

Adding is_bad_inode() check without corresponding iput() in error path causes
an inode leak bug. Also, error code will differ (my patch returns -EIO while
your approach will return -EINVAL).

Honestly speaking, I don't like use of make_bad_inode(). make_bad_inode() might
change file type. Also, I worry that make_bad_inode() causes a subtle race bug
like https://syzkaller.appspot.com/bug?extid=b7c3ba8cdc2f6cf83c21 which has not
come to a conclusion.

Why can't we remove make_bad_inode() usage from hfs_read_inode() and return non-0 value
(so that inode_insert5() will return NULL and iget5_locked() will call destroy_inode()
and return NULL) when hfs_read_inode() encountered an invalid entry?

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

[Viacheslav Dubeyko]

On Fri, 2025-11-14 at 23:18 +0900, Tetsuo Handa wrote:
> On 2025/11/12 7:56, Viacheslav Dubeyko wrote:
> > The file system is mounted only if hfs_fill_super() created root node and return
> > 0 [1]. However, if hfs_iget() return bad inode [2] and we will call
> > is_bad_inode() here [3]:
> > 
> > 	root_inode = hfs_iget(sb, &fd.search_key->cat, &rec);
> > 	hfs_find_exit(&fd);
> > 	if (!root_inode || is_bad_inode(root_inode)) <-- call will be here
> > 		goto bail_no_root;
> > 
> > then, mount will fail. So, no successful mount will happen because
> > is_valid_cnid() will manage the check in hfs_read_inode().
> 
> Do you admit that mounting (and optionally fuzzing on) a bad inode (an inode
> which was subjected to make_bad_inode()) is useless?
> 

Mount will fail in the case of bad root inode. I don't see any issue here.

> Adding is_bad_inode() check without corresponding iput() in error path causes
> an inode leak bug. Also, error code will differ (my patch returns -EIO while
> your approach will return -EINVAL).
> 

I don't see any problem to add iput(root_inode) as part of failure management in
hfs_fill_super(). And we can return any proper error code for the case of bad
root inode.

> Honestly speaking, I don't like use of make_bad_inode(). make_bad_inode() might
> change file type.
> 

If Catalog File's entry is corrupted, then we cannot assume which particular
file type is correct one. And nobody will operate by bad inode. So, it doesn't
matter which type bad inode has.

>  Also, I worry that make_bad_inode() causes a subtle race bug
> like https://syzkaller.appspot.com/bug?extid=b7c3ba8cdc2f6cf83c21   which has not
> come to a conclusion.
> 

Frankly speaking, I don't see how this issue is related to the HFS mount
operation.

> Why can't we remove make_bad_inode() usage from hfs_read_inode() and return non-0 value
> (so that inode_insert5() will return NULL and iget5_locked() will call destroy_inode()
> and return NULL) when hfs_read_inode() encountered an invalid entry?

Bunch of other file systems use the make_bad_inode() and in fill_super() call
too. I don't see what is wrong with calling make_bad_inode().

Thanks,
Slava.