On Wed, 2025-07-23 at 18:19 +0000, Viacheslav Dubeyko wrote:
> On Wed, 2025-07-23 at 11:16 +0900, Tetsuo Handa wrote:
> > With below change, legitimate HFS filesystem images can be mounted.
> >
> > But is crafted HFS filesystem images can not be mounted expected result?
> >
> > # losetup -a
> > /dev/loop0: [0001]:7185 (/memfd:syzkaller (deleted))
> > # mount -t hfs /dev/loop0 /mnt/
> > mount: /mnt: filesystem was mounted, but any subsequent operation failed: Operation not permitted.
> > # fsck.hfs /dev/loop0
> > ** /dev/loop0
> > Executing fsck_hfs (version 540.1-Linux).
> > ** Checking HFS volume.
> > Invalid extent entry
> > (3, 0)
> > ** The volume could not be verified completely.
> > # mount -t hfs /dev/loop0 /mnt/
> > mount: /mnt: filesystem was mounted, but any subsequent operation failed: Operation not permitted.
> >
> > Also, are IDs which should be excluded from make_bad_inode() conditions
> > same for HFS_CDR_FIL and HFS_CDR_DIR ?
> >
> >
> > --- a/fs/hfs/inode.c
> > +++ b/fs/hfs/inode.c
> > @@ -358,6 +358,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
> > inode->i_op = &hfs_file_inode_operations;
> > inode->i_fop = &hfs_file_operations;
> > inode->i_mapping->a_ops = &hfs_aops;
> > + if (inode->i_ino < HFS_FIRSTUSER_CNID)
> > + goto check_reserved_ino;
> > break;
> > case HFS_CDR_DIR:
> > inode->i_ino = be32_to_cpu(rec->dir.DirID);
> > @@ -368,6 +370,24 @@ static int hfs_read_inode(struct inode *inode, void *data)
> > inode_set_atime_to_ts(inode, inode_set_ctime_to_ts(inode, hfs_m_to_utime(rec->dir.MdDat))));
> > inode->i_op = &hfs_dir_inode_operations;
> > inode->i_fop = &hfs_dir_operations;
> > + if (inode->i_ino < HFS_FIRSTUSER_CNID)
> > + goto check_reserved_ino;
> > + break;
> > + default:
> > + make_bad_inode(inode);
> > + }
> > + return 0;
> > +check_reserved_ino:
> > + switch (inode->i_ino) {
> > + case HFS_POR_CNID:
> > + case HFS_ROOT_CNID:
> > + case HFS_EXT_CNID:
> > + case HFS_CAT_CNID:
> > + case HFS_BAD_CNID:
> > + case HFS_ALLOC_CNID:
> > + case HFS_START_CNID:
> > + case HFS_ATTR_CNID:
> > + case HFS_EXCH_CNID:
> > break;
> > default:
> > make_bad_inode(inode);
>
> I have missed that this list contains [1]:
>
> #define HFS_POR_CNID 1 /* Parent Of the Root */
> #define HFS_ROOT_CNID 2 /* ROOT directory */
>
> Of course, 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.
> However, the other constants cannot be used because they should be described in
> superblock (MDB) and Catalog File cannot have the records for them.
>
I have checked the HFS specification. So, additional corrections:
#define HFS_POR_CNID 1 /* Parent Of the Root */
#define HFS_ROOT_CNID 2 /* ROOT directory */
These values are legitimate values.
#define HFS_EXT_CNID 3 /* EXTents B-tree */
#define HFS_CAT_CNID 4 /* CATalog B-tree */
This metadata structures are defined in MDB.
#define HFS_BAD_CNID 5 /* BAD blocks file */
This could be defined in Catalog File because MDB has nothing for this metadata
structure. However, it's ancient technology.
#define HFS_ALLOC_CNID 6 /* ALLOCation file (HFS+) */
#define HFS_START_CNID 7 /* STARTup file (HFS+) */
#define HFS_ATTR_CNID 8 /* ATTRibutes file (HFS+) */
These value are invalid for HFS.
#define HFS_EXCH_CNID 15 /* ExchangeFiles temp id */
This could be defined in Catalog File (maybe not). I didn't find anything
related to this in HFS specification.
> Thanks,
> Slava.
>
> [1] https://elixir.bootlin.com/linux/v6.16-rc7/source/fs/hfs/hfs.h#L41
