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

[Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>]

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