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

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

[Tetsuo Handa]

The inode mode loaded from corrupted disk can be invalid. Do like what
commit 0a9e74051313 ("isofs: Verify inode mode when loading from disk")
does.

Reported-by: syzbot <syzbot+895c23f6917da440ed0d@syzkaller.appspotmail.com>
Closes: https://syzkaller.appspot.com/bug?extid=895c23f6917da440ed0d
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 fs/hfsplus/inode.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
index b51a411ecd23..53f653019904 100644
--- a/fs/hfsplus/inode.c
+++ b/fs/hfsplus/inode.c
@@ -558,9 +558,15 @@ int hfsplus_cat_read_inode(struct inode *inode, struct hfs_find_data *fd)
 			inode->i_op = &page_symlink_inode_operations;
 			inode_nohighmem(inode);
 			inode->i_mapping->a_ops = &hfsplus_aops;
-		} else {
+		} else if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode) ||
+			   S_ISFIFO(inode->i_mode) || S_ISSOCK(inode->i_mode)) {
 			init_special_inode(inode, inode->i_mode,
 					   be32_to_cpu(file->permissions.dev));
+		} else {
+			printk(KERN_DEBUG "hfsplus: Invalid file type 0%04o for inode %lu.\n",
+			       inode->i_mode, inode->i_ino);
+			res = -EIO;
+			goto out;
 		}
 		inode_set_atime_to_ts(inode, hfsp_mt2ut(file->access_date));
 		inode_set_mtime_to_ts(inode,
-- 
2.47.3

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

[Viacheslav Dubeyko]

On Sat, 2025-10-04 at 22:34 +0900, Tetsuo Handa wrote:
> The inode mode loaded from corrupted disk can be invalid. Do like
> what
> commit 0a9e74051313 ("isofs: Verify inode mode when loading from
> disk")
> does.
> 
> Reported-by: syzbot
> <syzbot+895c23f6917da440ed0d@syzkaller.appspotmail.com>
> Closes: https://syzkaller.appspot.com/bug?extid=895c23f6917da440ed0d
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  fs/hfsplus/inode.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
> index b51a411ecd23..53f653019904 100644
> --- a/fs/hfsplus/inode.c
> +++ b/fs/hfsplus/inode.c
> @@ -558,9 +558,15 @@ int hfsplus_cat_read_inode(struct inode *inode,
> struct hfs_find_data *fd)
>  			inode->i_op =
> &page_symlink_inode_operations;
>  			inode_nohighmem(inode);
>  			inode->i_mapping->a_ops = &hfsplus_aops;
> -		} else {
> +		} else if (S_ISCHR(inode->i_mode) || S_ISBLK(inode-
> >i_mode) ||
> +			   S_ISFIFO(inode->i_mode) ||
> S_ISSOCK(inode->i_mode)) {

As far as I can see, we operate by inode->i_mode here. But if inode
mode has been corrupted on disk, then we assigned wrong value before.
And HFS+ has hfsplus_get_perms() method that assigns perms->mode to
inode->i_mode. So, I think we need to rework hfsplus_get_perms() for
checking the correctness of inode mode before assigning it to inode-
>i_mode.

Thanks,
Slava.

>  			init_special_inode(inode, inode->i_mode,
>  					   be32_to_cpu(file-
> >permissions.dev));
> +		} else {
> +			printk(KERN_DEBUG "hfsplus: Invalid file
> type 0%04o for inode %lu.\n",
> +			       inode->i_mode, inode->i_ino);
> +			res = -EIO;
> +			goto out;
>  		}
>  		inode_set_atime_to_ts(inode, hfsp_mt2ut(file-
> >access_date));
>  		inode_set_mtime_to_ts(inode,

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

[Tetsuo Handa]

On 2025/10/07 23:22, Viacheslav Dubeyko wrote:
>> @@ -558,9 +558,15 @@ int hfsplus_cat_read_inode(struct inode *inode,
>> struct hfs_find_data *fd)
>>  			inode->i_op =
>> &page_symlink_inode_operations;
>>  			inode_nohighmem(inode);
>>  			inode->i_mapping->a_ops = &hfsplus_aops;
>> -		} else {
>> +		} else if (S_ISCHR(inode->i_mode) || S_ISBLK(inode-
>>> i_mode) ||
>> +			   S_ISFIFO(inode->i_mode) ||
>> S_ISSOCK(inode->i_mode)) {
> 
> As far as I can see, we operate by inode->i_mode here. But if inode
> mode has been corrupted on disk, then we assigned wrong value before.
> And HFS+ has hfsplus_get_perms() method that assigns perms->mode to
> inode->i_mode. So, I think we need to rework hfsplus_get_perms() for
> checking the correctness of inode mode before assigning it to inode->
> i_mode.

Then, can you give us an authoritative explanation, with historical part
fully understood?



While we can change the return value of hfsplus_get_perms() from "void" to
"int", the intent of what hfsplus_get_perms() wants to accept is unclear.

Commit bc107a619f02 ("squashfs: verify inode mode when loading from disk") was
clear because there is a format specification explaining that the value of the
permission field must not contain file type.

But what hfsplus_get_perms() is doing is quite puzzling, possibly due to historical
reasons which I don't know. (I'm not a Mac user.)
https://developer.apple.com/library/archive/technotes/tn/tn1150.html#HFSPlusPermissions
has a bit of explanation about MacOS version dependent behavior which are more than 25
years ago. I don't know whether the historical part (case for 0 value intentionally
stored) and fuzz testings (cases for random values intentional stored) are compatible.
Maybe we should discard historical part if you want to rework hfsplus_get_perms() for
checking the correctness of inode mode...


When dir == 1 (i.e. hfs_bnode_read_u16() returned HFSPLUS_FOLDER),
the S_IFMT field is unconditionally set to S_IFDIR, for

  mode = mode ? (mode & S_IALLUGO) : (S_IRWXUGO & ~(sbi->umask))

always makes the S_IFMT field == 0 and therefore

  mode |= S_IFDIR;

always makes the S_IFMT field == S_IFDIR. But this means that the S_IFMT
field is set to S_IFDIR even if the S_IFMT field in "mode" is not S_IFDIR
(either 0 or some random values). What is the intent of this "ignore the
S_IFMT field" ? Were old MacOS versions writing to disk with the S_IFMT
field == 0 even if the created file was a directory? But how can we tell
whether the value is correct (legitimate 0 due to historical reason or
invalid some random values due to fuzz testing) ?

When dir == 0 (i.e. hfs_bnode_read_u16() returned HFSPLUS_FILE), the S_IFMT
field is set to S_IFREG only when file->permissions.mode == 0. That is, if
file->permissions.mode != 0, we are currently unconditionally trusting that
the S_IFMT field is set to one of S_IFLNK/S_IFREG/S_IFCHR/S_IFBLK/S_IFIFO/
S_IFSOCK. This allows fuzzers to assign random S_IFMT field values (including
S_IFDIR itself) when hfs_bnode_read_u16() returned HFSPLUS_FILE. But how can we
tell whether the value is correct (legitimate 0 due to historical reason or
invalid some random values due to fuzz testing) ?

My patch simply checks that the result of hfsplus_get_perms() is one of
S_IFLNK/S_IFREG/S_IFCHR/S_IFBLK/S_IFIFO/S_IFSOCK, by ignoring whether the
value is legitimate 0 or not.


Also, it seems that the HFS+ filesystem defines S_IFWHT file type which is not
supported by Linux. Commit 4e8011ffec79 ("ntfs3: pretend $Extend records as
regular files") decided to assign S_IFREG for irregular files where the file
type was previously 0, for operations such as read(), truncate() are not
possible even if S_IFREG is assigned. But when HFS+ filesystem is mounted in
Linux environment, what file type should we assign for HFS+ files which have
S_IFWHT file type? Can we simply return -EIO for S_IFWHT files?

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

[Tetsuo Handa]

Ping?

Now that BFS got
https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/commit/?h=vfs.fixes&id=34ab4c75588c07cca12884f2bf6b0347c7a13872 ,
HFS+ became the last filesystem that has not come to an answer.

On 2025/10/08 20:21, Tetsuo Handa wrote:
>> As far as I can see, we operate by inode->i_mode here. But if inode
>> mode has been corrupted on disk, then we assigned wrong value before.
>> And HFS+ has hfsplus_get_perms() method that assigns perms->mode to
>> inode->i_mode. So, I think we need to rework hfsplus_get_perms() for
>> checking the correctness of inode mode before assigning it to inode->
>> i_mode.
> 
> Then, can you give us an authoritative explanation, with historical part
> fully understood?

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

[Viacheslav Dubeyko]

On Sat, 2025-11-15 at 00:35 +0900, Tetsuo Handa wrote:
> Ping?
> 

You checks the inode->i_mode in the patch. And logical place for this
check is hfsplus_get_perms() method [1] because all mode checks are
made there before assigning to inode->i_mode. So, you simply need to
add this check there and to check the return value of
hfsplus_get_perms() in hfsplus_cat_read_inode().

static void 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);

<skipped>

	if (dir) {
		mode = mode ? (mode & S_IALLUGO) : (S_IRWXUGO & ~(sbi-
>umask));
		mode |= S_IFDIR;
	} else if (!mode)
		mode = S_IFREG | ((S_IRUGO|S_IWUGO) & ~(sbi->umask));

<-- You simply can implement your check here.

	inode->i_mode = mode;

<skipped>
}

It simple modification from my point of view. And I don't quite follow
to your difficulties to move your check into hfsplus_get_perms().

Thanks,
Slava.

[1]
https://elixir.bootlin.com/linux/v6.18-rc5/source/fs/hfsplus/inode.c#L183

> Now that BFS got
> https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/commit/?h=vfs.fixes&id=34ab4c75588c07cca12884f2bf6b0347c7a13872
>  ,
> HFS+ became the last filesystem that has not come to an answer.
> 
> On 2025/10/08 20:21, Tetsuo Handa wrote:
> > > As far as I can see, we operate by inode->i_mode here. But if
> > > inode
> > > mode has been corrupted on disk, then we assigned wrong value
> > > before.
> > > And HFS+ has hfsplus_get_perms() method that assigns perms->mode
> > > to
> > > inode->i_mode. So, I think we need to rework hfsplus_get_perms()
> > > for
> > > checking the correctness of inode mode before assigning it to
> > > inode->
> > > i_mode.
> > 
> > Then, can you give us an authoritative explanation, with historical
> > part
> > fully understood?

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

[Tetsuo Handa]

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)
-- 
2.47.3

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

[Viacheslav Dubeyko]

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.