Re: [PATCH] hfs: Validate CNIDs in hfs_read_inode

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

On Fri, 2025-10-03 at 03:45 +0100, George Anthony Vernon wrote:
> hfs_read_inode previously did not validate CNIDs read from disk, thereby
> allowing inodes to be constructed with disallowed CNIDs and placed on
> the dirty list, eventually hitting a bug on writeback.
> 
> Validate reserved CNIDs according to Apple technical note TN1150.
> 
> This issue was discussed at length on LKML previously, the discussion
> is linked below.
> 
> Syzbot tested this patch on mainline and the bug did not replicate.
> This patch was regression tested by issuing various system calls on a
> mounted HFS filesystem and validating that file creation, deletion,
> reads and writes all work.
> 
> Link: https://lore.kernel.org/all/427fcb57-8424-4e52-9f21-7041b2c4ae5b@  
> I-love.SAKURA.ne.jp/T/
> Reported-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b  
> Tested-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com

Let's pay respect to previous efforts. I am suggesting to add this line:

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Are you OK with it?

> Signed-off-by: George Anthony Vernon <contact@gvernon.com>
> ---
>  fs/hfs/inode.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
> index 9cd449913dc8..6f893011492a 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -321,6 +321,30 @@ static int hfs_test_inode(struct inode *inode, void *data)
>  	}
>  }
>  
> +/*
> + * is_valid_cnid
> + *
> + * Validate the CNID of a catalog record read from disk
> + */
> +static bool is_valid_cnid(unsigned long cnid, s8 type)

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.

Why type has signed type (s8)? We don't expect negative values here. Let's use
u8 type.

> +{
> +	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).

> +}
> +
>  /*
>   * hfs_read_inode
>   */
> @@ -359,6 +383,11 @@ static int hfs_read_inode(struct inode *inode, void *data)
>  		}
>  
>  		inode->i_ino = be32_to_cpu(rec->file.FlNum);
> +		if (!is_valid_cnid(inode->i_ino, HFS_CDR_FIL)) {
> +			pr_warn("rejected cnid %lu\n", inode->i_ino);

The syzbot reported the issue on specially corrupted volume. So, probably, it
will be better to mentioned that "volume is probably corrupted" and to advice to
run FSCK tool.

> +			make_bad_inode(inode);

We already have make_bad_inode(inode) under default case of switch. Let's jump
there without replicating this call multiple times. It makes the code more
complicated.

> +			break;
> +		}
>  		inode->i_mode = S_IRUGO | S_IXUGO;
>  		if (!(rec->file.Flags & HFS_FIL_LOCK))
>  			inode->i_mode |= S_IWUGO;
> @@ -372,6 +401,11 @@ static int hfs_read_inode(struct inode *inode, void *data)
>  		break;
>  	case HFS_CDR_DIR:
>  		inode->i_ino = be32_to_cpu(rec->dir.DirID);
> +		if (!is_valid_cnid(inode->i_ino, HFS_CDR_DIR)) {
> +			pr_warn("rejected cnid %lu\n", inode->i_ino);

Ditto.

> +			make_bad_inode(inode);

Ditto.

> +			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? 

		}
	}

<skipped>
}

What's about to use your check here too?

Mostly, I like your approach but the patch needs some polishing yet. ;)

Thanks,
Slava.