fs/hfs/super.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
syzbot is reporting that BUG() in hfs_write_inode() fires upon unmount
operation when the inode number of the record retrieved as a result of
hfs_cat_find_brec(HFS_ROOT_CNID) is not HFS_ROOT_CNID, for
commit b905bafdea21 ("hfs: Sanity check the root record") checked
the record size and the record type but did not check the inode number.
Reported-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
fs/hfs/super.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/hfs/super.c b/fs/hfs/super.c
index fe09c2093a93..d231989b4e23 100644
--- a/fs/hfs/super.c
+++ b/fs/hfs/super.c
@@ -354,7 +354,7 @@ static int hfs_fill_super(struct super_block *sb, struct fs_context *fc)
goto bail_hfs_find;
}
hfs_bnode_read(fd.bnode, &rec, fd.entryoffset, fd.entrylength);
- if (rec.type != HFS_CDR_DIR)
+ if (rec.type != HFS_CDR_DIR || rec.dir.DirID != cpu_to_be32(HFS_ROOT_CNID))
res = -EIO;
}
if (res)
--
2.50.1On Wed, 2025-07-30 at 08:21 +0900, Tetsuo Handa wrote:
> syzbot is reporting that BUG() in hfs_write_inode() fires upon unmount
> operation when the inode number of the record retrieved as a result of
> hfs_cat_find_brec(HFS_ROOT_CNID) is not HFS_ROOT_CNID, for
> commit b905bafdea21 ("hfs: Sanity check the root record") checked
> the record size and the record type but did not check the inode number.
>
> Reported-by: syzbot+97e301b4b82ae803d21b@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> fs/hfs/super.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/hfs/super.c b/fs/hfs/super.c
> index fe09c2093a93..d231989b4e23 100644
> --- a/fs/hfs/super.c
> +++ b/fs/hfs/super.c
> @@ -354,7 +354,7 @@ static int hfs_fill_super(struct super_block *sb, struct fs_context *fc)
> goto bail_hfs_find;
> }
> hfs_bnode_read(fd.bnode, &rec, fd.entryoffset, fd.entrylength);
> - if (rec.type != HFS_CDR_DIR)
> + if (rec.type != HFS_CDR_DIR || rec.dir.DirID != cpu_to_be32(HFS_ROOT_CNID))
> res = -EIO;
> }
> if (res)
Why do not localize the all checks in hfs_read_inode()?
We will do such logic then [1], even if rec.dir.DirID !=
cpu_to_be32(HFS_ROOT_CNID):
root_inode = hfs_iget(sb, &fd.search_key->cat, &rec);
hfs_find_exit(&fd);
if (!root_inode)
goto bail_no_root;
The hfs_iget() calls iget5_locked() [2]:
inode = iget5_locked(sb, cnid, hfs_test_inode, hfs_read_inode, &data);
And hfs_read_inode() will be called, finally. If inode ID is wrong, then
make_bad_inode(inode) can be called [3].
If we considering case HFS_CDR_DIR in hfs_read_inode(), then we know that it
could be HFS_POR_CNID, HFS_ROOT_CNID, or >= HFS_FIRSTUSER_CNID. Do you mean that
HFS_POR_CNID could be a problem in hfs_write_inode()?
Thanks,
Slava.
[1] https://elixir.bootlin.com/linux/v6.16-rc6/source/fs/hfs/super.c#L363
[2] https://elixir.bootlin.com/linux/v6.16-rc6/source/fs/hfs/inode.c#L403
[3]
https://elixir.bootlin.com/linux/v6.16-rc6/source/fs/hfs/inode.c#L373
On 2025/07/31 4:24, Viacheslav Dubeyko wrote: > If we considering case HFS_CDR_DIR in hfs_read_inode(), then we know that it > could be HFS_POR_CNID, HFS_ROOT_CNID, or >= HFS_FIRSTUSER_CNID. Do you mean that > HFS_POR_CNID could be a problem in hfs_write_inode()? Yes. Passing one of 1, 5 or 15 instead of 2 from hfs_fill_super() triggers BUG() in hfs_write_inode(). We *MUST* validate at hfs_fill_super(), or hfs_read_inode() shall have to also reject 1, 5 and 15 (and as a result only accept 2).
On Thu, 2025-07-31 at 07:02 +0900, Tetsuo Handa wrote: > On 2025/07/31 4:24, Viacheslav Dubeyko wrote: > > If we considering case HFS_CDR_DIR in hfs_read_inode(), then we know that it > > could be HFS_POR_CNID, HFS_ROOT_CNID, or >= HFS_FIRSTUSER_CNID. Do you mean that > > HFS_POR_CNID could be a problem in hfs_write_inode()? > > Yes. Passing one of 1, 5 or 15 instead of 2 from hfs_fill_super() triggers BUG() > in hfs_write_inode(). We *MUST* validate at hfs_fill_super(), or hfs_read_inode() > shall have to also reject 1, 5 and 15 (and as a result only accept 2). The fix should be in hfs_read_inode(). Currently, suggested solution hides the issue but not fix the problem. Because b-tree nodes could contain multiple corrupted records. Now, this patch checks only record for root folder. Let's imagine that root folder record will be OK but another record(s) will be corrupted in such way. Finally, we will have successful mount but operation with corrupted record(s) will trigger this issue. So, I cannot consider this patch as a complete fix of the problem. Thanks, Slava.
On 2025/08/01 3:03, Viacheslav Dubeyko wrote:
> On Thu, 2025-07-31 at 07:02 +0900, Tetsuo Handa wrote:
>> On 2025/07/31 4:24, Viacheslav Dubeyko wrote:
>>> If we considering case HFS_CDR_DIR in hfs_read_inode(), then we know that it
>>> could be HFS_POR_CNID, HFS_ROOT_CNID, or >= HFS_FIRSTUSER_CNID. Do you mean that
>>> HFS_POR_CNID could be a problem in hfs_write_inode()?
>>
>> Yes. Passing one of 1, 5 or 15 instead of 2 from hfs_fill_super() triggers BUG()
>> in hfs_write_inode(). We *MUST* validate at hfs_fill_super(), or hfs_read_inode()
>> shall have to also reject 1, 5 and 15 (and as a result only accept 2).
>
> The fix should be in hfs_read_inode(). Currently, suggested solution hides the
> issue but not fix the problem.
Not fixing this problem might be hiding other issues, by hitting BUG() before
other issues shows up.
> Because b-tree nodes could contain multiple
> corrupted records. Now, this patch checks only record for root folder. Let's
> imagine that root folder record will be OK but another record(s) will be
> corrupted in such way.
Can the inode number of the record retrieved as a result of
hfs_cat_find_brec(HFS_ROOT_CNID) be something other than HFS_ROOT_CNID ?
If the inode number of the record retrieved as a result of
hfs_cat_find_brec(HFS_ROOT_CNID) must be HFS_ROOT_CNID, this patch itself will be
a complete fix for this problem.
> Finally, we will have successful mount but operation with
> corrupted record(s) will trigger this issue. So, I cannot consider this patch as
> a complete fix of the problem.
Did you try what you think as a fix of this problem (I guess something like
shown below will be needed for avoid hitting BUG()) using
https://lkml.kernel.org/r/a8f8da77-f099-499b-98e0-39ed159b6a2d@I-love.SAKURA.ne.jp ?
diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index a81ce7a740b9..d60395111ed5 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -81,7 +81,8 @@ static bool hfs_release_folio(struct folio *folio, gfp_t mask)
tree = HFS_SB(sb)->cat_tree;
break;
default:
- BUG();
+ pr_err("detected unknown inode %lu, running fsck.hfs is recommended.\n",
+ inode->i_ino);
return false;
}
@@ -305,11 +306,31 @@ static int hfs_test_inode(struct inode *inode, void *data)
case HFS_CDR_FIL:
return inode->i_ino == be32_to_cpu(rec->file.FlNum);
default:
- BUG();
+ pr_err("detected unknown type %u, running fsck.hfs is recommended.\n", rec->type);
return 1;
}
}
+static bool is_bad_id(unsigned long ino)
+{
+ switch (ino) {
+ case 0:
+ case 3:
+ case 4:
+ case 6:
+ case 7:
+ case 8:
+ case 9:
+ case 10:
+ case 11:
+ case 12:
+ case 13:
+ case 14:
+ return true;
+ }
+ return false;
+}
+
/*
* hfs_read_inode
*/
@@ -348,6 +369,10 @@ static int hfs_read_inode(struct inode *inode, void *data)
}
inode->i_ino = be32_to_cpu(rec->file.FlNum);
+ if (is_bad_id(inode->i_ino)) {
+ make_bad_inode(inode);
+ break;
+ }
inode->i_mode = S_IRUGO | S_IXUGO;
if (!(rec->file.Flags & HFS_FIL_LOCK))
inode->i_mode |= S_IWUGO;
@@ -358,9 +383,15 @@ 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 < 16)
+ pr_info("HFS_CDR_FIL i_ino=%ld\n", inode->i_ino);
break;
case HFS_CDR_DIR:
inode->i_ino = be32_to_cpu(rec->dir.DirID);
+ if (is_bad_id(inode->i_ino)) {
+ make_bad_inode(inode);
+ 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);
@@ -368,6 +399,8 @@ 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 < 16)
+ pr_info("HFS_CDR_DIR i_ino=%ld\n", inode->i_ino);
break;
default:
make_bad_inode(inode);
@@ -441,7 +474,8 @@ int hfs_write_inode(struct inode *inode, struct writeback_control *wbc)
hfs_btree_write(HFS_SB(inode->i_sb)->cat_tree);
return 0;
default:
- BUG();
+ pr_err("detected unknown inode %lu, running fsck.hfs is recommended.\n",
+ inode->i_ino);
return -EIO;
}
}
# for i in $(seq 0 15); do timeout 1 unshare -m ./hfs $i; done
# dmesg | grep fsck
[ 52.563547] [ T479] hfs: detected unknown inode 1, running fsck.hfs is recommended.
[ 56.606238] [ T255] hfs: detected unknown inode 5, running fsck.hfs is recommended.
[ 66.694795] [ T500] hfs: detected unknown inode 15, running fsck.hfs is recommended.
On Fri, 2025-08-01 at 06:12 +0900, Tetsuo Handa wrote:
> On 2025/08/01 3:03, Viacheslav Dubeyko wrote:
> > On Thu, 2025-07-31 at 07:02 +0900, Tetsuo Handa wrote:
> > > On 2025/07/31 4:24, Viacheslav Dubeyko wrote:
> > > > If we considering case HFS_CDR_DIR in hfs_read_inode(), then we know that it
> > > > could be HFS_POR_CNID, HFS_ROOT_CNID, or >= HFS_FIRSTUSER_CNID. Do you mean that
> > > > HFS_POR_CNID could be a problem in hfs_write_inode()?
> > >
> > > Yes. Passing one of 1, 5 or 15 instead of 2 from hfs_fill_super() triggers BUG()
> > > in hfs_write_inode(). We *MUST* validate at hfs_fill_super(), or hfs_read_inode()
> > > shall have to also reject 1, 5 and 15 (and as a result only accept 2).
> >
> > The fix should be in hfs_read_inode(). Currently, suggested solution hides the
> > issue but not fix the problem.
>
> Not fixing this problem might be hiding other issues, by hitting BUG() before
> other issues shows up.
>
I am not going to start a philosophical discussion. We simply need to fix the
bug. The suggested patch doesn't fix the issue.
> > Because b-tree nodes could contain multiple
> > corrupted records. Now, this patch checks only record for root folder. Let's
> > imagine that root folder record will be OK but another record(s) will be
> > corrupted in such way.
>
> Can the inode number of the record retrieved as a result of
> hfs_cat_find_brec(HFS_ROOT_CNID) be something other than HFS_ROOT_CNID ?
>
> If the inode number of the record retrieved as a result of
> hfs_cat_find_brec(HFS_ROOT_CNID) must be HFS_ROOT_CNID, this patch itself will be
> a complete fix for this problem.
>
You are working with corrupted volume. In this case, you can extract any state
of the Catalog File's record.
> > Finally, we will have successful mount but operation with
> > corrupted record(s) will trigger this issue. So, I cannot consider this patch as
> > a complete fix of the problem.
>
> Did you try what you think as a fix of this problem (I guess something like
> shown below will be needed for avoid hitting BUG()) using
> https://lkml.kernel.org/r/a8f8da77-f099-499b-98e0-39ed159b6a2d@I-love.SAKURA.ne.jp ?
>
If you believe that you have another version of the patch, then simply send it
and I will review it. Sorry, I haven't enough time to discuss every movement of
your thoughts.
> diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
> index a81ce7a740b9..d60395111ed5 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -81,7 +81,8 @@ static bool hfs_release_folio(struct folio *folio, gfp_t mask)
> tree = HFS_SB(sb)->cat_tree;
> break;
> default:
> - BUG();
> + pr_err("detected unknown inode %lu, running fsck.hfs is recommended.\n",
> + inode->i_ino);
> return false;
> }
>
> @@ -305,11 +306,31 @@ static int hfs_test_inode(struct inode *inode, void *data)
> case HFS_CDR_FIL:
> return inode->i_ino == be32_to_cpu(rec->file.FlNum);
> default:
> - BUG();
> + pr_err("detected unknown type %u, running fsck.hfs is recommended.\n", rec->type);
> return 1;
> }
> }
>
> +static bool is_bad_id(unsigned long ino)
> +{
> + switch (ino) {
> + case 0:
> + case 3:
> + case 4:
> + case 6:
> + case 7:
> + case 8:
> + case 9:
> + case 10:
> + case 11:
> + case 12:
> + case 13:
> + case 14:
> + return true;
> + }
> + return false;
> +}
Please, don't use hardcoded value. I already shared the point that we must use
the declared constants.
This function is incorrect and it cannot work for folders and files at the same
time.
Thanks,
Slava.
> +
> /*
> * hfs_read_inode
> */
> @@ -348,6 +369,10 @@ static int hfs_read_inode(struct inode *inode, void *data)
> }
>
> inode->i_ino = be32_to_cpu(rec->file.FlNum);
> + if (is_bad_id(inode->i_ino)) {
> + make_bad_inode(inode);
> + break;
> + }
> inode->i_mode = S_IRUGO | S_IXUGO;
> if (!(rec->file.Flags & HFS_FIL_LOCK))
> inode->i_mode |= S_IWUGO;
> @@ -358,9 +383,15 @@ 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 < 16)
> + pr_info("HFS_CDR_FIL i_ino=%ld\n", inode->i_ino);
> break;
> case HFS_CDR_DIR:
> inode->i_ino = be32_to_cpu(rec->dir.DirID);
> + if (is_bad_id(inode->i_ino)) {
> + make_bad_inode(inode);
> + 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);
> @@ -368,6 +399,8 @@ 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 < 16)
> + pr_info("HFS_CDR_DIR i_ino=%ld\n", inode->i_ino);
> break;
> default:
> make_bad_inode(inode);
> @@ -441,7 +474,8 @@ int hfs_write_inode(struct inode *inode, struct writeback_control *wbc)
> hfs_btree_write(HFS_SB(inode->i_sb)->cat_tree);
> return 0;
> default:
> - BUG();
> + pr_err("detected unknown inode %lu, running fsck.hfs is recommended.\n",
> + inode->i_ino);
> return -EIO;
> }
> }
>
>
> # for i in $(seq 0 15); do timeout 1 unshare -m ./hfs $i; done
> # dmesg | grep fsck
> [ 52.563547] [ T479] hfs: detected unknown inode 1, running fsck.hfs is recommended.
> [ 56.606238] [ T255] hfs: detected unknown inode 5, running fsck.hfs is recommended.
> [ 66.694795] [ T500] hfs: detected unknown inode 15, running fsck.hfs is recommended.
On 2025/08/02 3:26, Viacheslav Dubeyko wrote: > On Fri, 2025-08-01 at 06:12 +0900, Tetsuo Handa wrote: >> On 2025/08/01 3:03, Viacheslav Dubeyko wrote: >>> On Thu, 2025-07-31 at 07:02 +0900, Tetsuo Handa wrote: >>>> On 2025/07/31 4:24, Viacheslav Dubeyko wrote: >>>>> If we considering case HFS_CDR_DIR in hfs_read_inode(), then we know that it >>>>> could be HFS_POR_CNID, HFS_ROOT_CNID, or >= HFS_FIRSTUSER_CNID. Do you mean that >>>>> HFS_POR_CNID could be a problem in hfs_write_inode()? >>>> >>>> Yes. Passing one of 1, 5 or 15 instead of 2 from hfs_fill_super() triggers BUG() >>>> in hfs_write_inode(). We *MUST* validate at hfs_fill_super(), or hfs_read_inode() >>>> shall have to also reject 1, 5 and 15 (and as a result only accept 2). >>> >>> The fix should be in hfs_read_inode(). Currently, suggested solution hides the >>> issue but not fix the problem. >> >> Not fixing this problem might be hiding other issues, by hitting BUG() before >> other issues shows up. >> > > I am not going to start a philosophical discussion. We simply need to fix the > bug. The suggested patch doesn't fix the issue. What is your issue? My issue (what syzbot is reporting) is that the kernel crashes if the inode number of the record retrieved as a result of hfs_cat_find_brec(HFS_ROOT_CNID) is not HFS_ROOT_CNID. My suggested patch does fix my issue. > Please, don't use hardcoded value. I already shared the point that we must use > the declared constants. > > This function is incorrect and it cannot work for folders and files at the same > time. I already shared that I don't plan to try writing such function ( http://lkml.kernel.org/r/38d8f48e-47c3-4d67-9caa-498f3b47004f@I-love.SAKURA.ne.jp ). Please show us your patch that solves your issue.
On Sat, 2025-08-02 at 06:52 +0900, Tetsuo Handa wrote: > On 2025/08/02 3:26, Viacheslav Dubeyko wrote: > > On Fri, 2025-08-01 at 06:12 +0900, Tetsuo Handa wrote: > > > On 2025/08/01 3:03, Viacheslav Dubeyko wrote: > > > > On Thu, 2025-07-31 at 07:02 +0900, Tetsuo Handa wrote: > > > > > On 2025/07/31 4:24, Viacheslav Dubeyko wrote: > > > > > > If we considering case HFS_CDR_DIR in hfs_read_inode(), then we know that it > > > > > > could be HFS_POR_CNID, HFS_ROOT_CNID, or >= HFS_FIRSTUSER_CNID. Do you mean that > > > > > > HFS_POR_CNID could be a problem in hfs_write_inode()? > > > > > > > > > > Yes. Passing one of 1, 5 or 15 instead of 2 from hfs_fill_super() triggers BUG() > > > > > in hfs_write_inode(). We *MUST* validate at hfs_fill_super(), or hfs_read_inode() > > > > > shall have to also reject 1, 5 and 15 (and as a result only accept 2). > > > > > > > > The fix should be in hfs_read_inode(). Currently, suggested solution hides the > > > > issue but not fix the problem. > > > > > > Not fixing this problem might be hiding other issues, by hitting BUG() before > > > other issues shows up. > > > > > > > I am not going to start a philosophical discussion. We simply need to fix the > > bug. The suggested patch doesn't fix the issue. > > What is your issue? > > My issue (what syzbot is reporting) is that the kernel crashes if the inode number > of the record retrieved as a result of hfs_cat_find_brec(HFS_ROOT_CNID) is not > HFS_ROOT_CNID. My suggested patch does fix my issue. > > > Please, don't use hardcoded value. I already shared the point that we must use > > the declared constants. > > > > This function is incorrect and it cannot work for folders and files at the same > > time. > > I already shared that I don't plan to try writing such function > ( http://lkml.kernel.org/r/38d8f48e-47c3-4d67-9caa-498f3b47004f@I-love.SAKURA.ne.jp ). > > Please show us your patch that solves your issue. OK. It will be faster to write my own patch. It works for me. Thanks, Slava.
On 2025/08/05 7:00, Viacheslav Dubeyko wrote:
>> Please show us your patch that solves your issue.
>
> OK. It will be faster to write my own patch. It works for me.
I haven't heard from you about your own patch.
I guess that your patch will include
diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index bf4cb7e78396..8d033ffeb8af 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -361,6 +361,10 @@ 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 (inode->i_ino < HFS_FIRSTUSER_CNID && inode->i_ino != HFS_ROOT_CNID) {
+ make_bad_inode(inode);
+ 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);
change, which results in the following.
----------
The root inode's i_ino is 0 or 1 = fail with EINVAL
The root inode's i_ino is 2 = success
The root inode's i_ino is 3 or 4 = fail with ENOTDIR
The root inode's i_ino is 5 to 15 = fail with EINVAL
The root inode's i_ino is 16 and beyond = success
----------
But my patch has extra validation on the root inode's i_ino,
which results in the following.
----------
The root inode's i_ino is 2 = success
The root inode's i_ino is all (i.e. including 16 and beyond) but 2 = fail with EIO
----------
Therefore, while you can propose your patch,
I consider that there is no reason to defer my patch.
© 2016 - 2025 Red Hat, Inc.