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

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

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