Re: [PATCH v2] hfsplus: Verify inode mode when loading from disk

[Viacheslav Dubeyko <slava@dubeyko.com>]

On Sat, 2025-11-15 at 18:18 +0900, Tetsuo Handa wrote:
> syzbot is reporting that S_IFMT bits of inode->i_mode can become
> bogus when
> the S_IFMT bits of the 16bits "mode" field loaded from disk are
> corrupted.
> 
> According to [1], the permissions field was treated as reserved in
> Mac OS
> 8 and 9. According to [2], the reserved field was explicitly
> initialized
> with 0, and that field must remain 0 as long as reserved. Therefore,
> when
> the "mode" field is not 0 (i.e. no longer reserved), the file must be
> S_IFDIR if dir == 1, and the file must be one of
> S_IFREG/S_IFLNK/S_IFCHR/
> S_IFBLK/S_IFIFO/S_IFSOCK if dir == 0.
> 
> Reported-by: syzbot
> <syzbot+895c23f6917da440ed0d@syzkaller.appspotmail.com>
> Closes: https://syzkaller.appspot.com/bug?extid=895c23f6917da440ed0d
> Link:
> https://developer.apple.com/library/archive/technotes/tn/tn1150.html#HFSPlusPermissions
>  [1]
> Link:
> https://developer.apple.com/library/archive/technotes/tn/tn1150.html#ReservedAndPadFields
>  [2]
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  fs/hfsplus/inode.c | 32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
> index b51a411ecd23..e290e417ed3a 100644
> --- a/fs/hfsplus/inode.c
> +++ b/fs/hfsplus/inode.c
> @@ -180,13 +180,29 @@ const struct dentry_operations
> hfsplus_dentry_operations = {
>  	.d_compare    = hfsplus_compare_dentry,
>  };
>  
> -static void hfsplus_get_perms(struct inode *inode,
> -		struct hfsplus_perm *perms, int dir)
> +static int hfsplus_get_perms(struct inode *inode,
> +			     struct hfsplus_perm *perms, int dir)
>  {
>  	struct hfsplus_sb_info *sbi = HFSPLUS_SB(inode->i_sb);
>  	u16 mode;
>  
>  	mode = be16_to_cpu(perms->mode);
> +	if (dir) {
> +		if (mode && !S_ISDIR(mode))
> +			goto bad_type;
> +	} else if (mode) {
> +		switch (mode & S_IFMT) {
> +		case S_IFREG:
> +		case S_IFLNK:
> +		case S_IFCHR:
> +		case S_IFBLK:
> +		case S_IFIFO:
> +		case S_IFSOCK:
> +			break;
> +		default:
> +			goto bad_type;
> +		}
> +	}
>  
>  	i_uid_write(inode, be32_to_cpu(perms->owner));
>  	if ((test_bit(HFSPLUS_SB_UID, &sbi->flags)) ||
> (!i_uid_read(inode) && !mode))
> @@ -212,6 +228,10 @@ static void hfsplus_get_perms(struct inode
> *inode,
>  		inode->i_flags |= S_APPEND;
>  	else
>  		inode->i_flags &= ~S_APPEND;
> +	return 0;
> +bad_type:
> +	pr_err("invalid file type 0%04o for inode %lu\n", mode,
> inode->i_ino);
> +	return -EIO;
>  }
>  
>  static int hfsplus_file_open(struct inode *inode, struct file *file)
> @@ -516,7 +536,9 @@ int hfsplus_cat_read_inode(struct inode *inode,
> struct hfs_find_data *fd)
>  		}
>  		hfs_bnode_read(fd->bnode, &entry, fd->entryoffset,
>  					sizeof(struct
> hfsplus_cat_folder));
> -		hfsplus_get_perms(inode, &folder->permissions, 1);
> +		res = hfsplus_get_perms(inode, &folder->permissions,
> 1);
> +		if (res)
> +			goto out;
>  		set_nlink(inode, 1);
>  		inode->i_size = 2 + be32_to_cpu(folder->valence);
>  		inode_set_atime_to_ts(inode, hfsp_mt2ut(folder-
> >access_date));
> @@ -545,7 +567,9 @@ int hfsplus_cat_read_inode(struct inode *inode,
> struct hfs_find_data *fd)
>  
>  		hfsplus_inode_read_fork(inode,
> HFSPLUS_IS_RSRC(inode) ?
>  					&file->rsrc_fork : &file-
> >data_fork);
> -		hfsplus_get_perms(inode, &file->permissions, 0);
> +		res = hfsplus_get_perms(inode, &file->permissions,
> 0);
> +		if (res)
> +			goto out;
>  		set_nlink(inode, 1);
>  		if (S_ISREG(inode->i_mode)) {
>  			if (file->permissions.dev)

Looks good.

Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>

Thanks,
Slava.