Message-ID: <6ec98658418f12b85e5161d28a59c48a68388b76.camel@dubeyko.com>
Date: Tue, 07 Oct 2025 06:40:50 -0700
From: Viacheslav Dubeyko <slava@...eyko.com>
To: George Anthony Vernon <contact@...rnon.com>, Viacheslav Dubeyko
	 <Slava.Dubeyko@....com>
Cc: "glaubitz@...sik.fu-berlin.de" <glaubitz@...sik.fu-berlin.de>, 
 "frank.li@...o.com"	 <frank.li@...o.com>, "skhan@...uxfoundation.org"
 <skhan@...uxfoundation.org>,  "linux-fsdevel@...r.kernel.org"	
 <linux-fsdevel@...r.kernel.org>, "linux-kernel-mentees@...ts.linux.dev"	
 <linux-kernel-mentees@...ts.linux.dev>, 
 "syzbot+97e301b4b82ae803d21b@...kaller.appspotmail.com"	
 <syzbot+97e301b4b82ae803d21b@...kaller.appspotmail.com>, 
 "linux-kernel@...r.kernel.org"	 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] hfs: Validate CNIDs in hfs_read_inode

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@...ove.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

Powered by blists - more mailing lists