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