Re: [PATCH] hfs: Validate CNIDs in hfs_read_inode

[George Anthony Vernon <contact@gvernon.com>]

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