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 the inode
number is assigned using 32bits integer retrieved from relevant offset of
the file system image.
Since commit b905bafdea21 ("hfs: Sanity check the root record") checks only
the record size and the record type, let's also check the inode number.
Since hfs_cat_find_brec() is called by only hfs_fill_super() with cnid ==
HFS_ROOT_CNID, we could move hfs_bnode_read() and related validations to
inside hfs_cat_find_brec(). But such change is a matter of preference
while this 3+ years old bug is effectively the top crasher for syzbot.
Let me immediately stop wasting syzbot's computation resource.
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>
---
George Anthony Vernon is trying to fix this bug without my patch
( https://lkml.kernel.org/r/20260311211309.27856-1-contact@gvernon.com ).
But it was proven that George's patch cannot fix this bug. Therefore,
I believe that we came to a conclusion that applying my patch is
the way to go. We can apply George's patch as a further improvement.
But unfortunately Viacheslav Dubeyko (the maintainer of HFS) is not
responding. Therefore, I'm sending this patch to more developers who
might accept this patch, instead of waiting for Viacheslav.
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 a4f2a2bfa6d3..2e52acf282b0 100644
--- a/fs/hfs/super.c
+++ b/fs/hfs/super.c
@@ -361,7 +361,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.53.0On Sat, 2026-03-28 at 16:12 +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 the inode
> number is assigned using 32bits integer retrieved from relevant offset of
> the file system image.
>
> Since commit b905bafdea21 ("hfs: Sanity check the root record") checks only
> the record size and the record type, let's also check the inode number.
>
> Since hfs_cat_find_brec() is called by only hfs_fill_super() with cnid ==
> HFS_ROOT_CNID, we could move hfs_bnode_read() and related validations to
> inside hfs_cat_find_brec(). But such change is a matter of preference
> while this 3+ years old bug is effectively the top crasher for syzbot.
> Let me immediately stop wasting syzbot's computation resource.
>
> 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>
> ---
> George Anthony Vernon is trying to fix this bug without my patch
> ( https://lkml.kernel.org/r/20260311211309.27856-1-contact@gvernon.com ).
> But it was proven that George's patch cannot fix this bug. Therefore,
> I believe that we came to a conclusion that applying my patch is
> the way to go. We can apply George's patch as a further improvement.
> But unfortunately Viacheslav Dubeyko (the maintainer of HFS) is not
> responding. Therefore, I'm sending this patch to more developers who
> might accept this patch, instead of waiting for Viacheslav.
It's completely not correct. I am reviewing patches in time when I have enough
free time for it.
>
> 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 a4f2a2bfa6d3..2e52acf282b0 100644
> --- a/fs/hfs/super.c
> +++ b/fs/hfs/super.c
> @@ -361,7 +361,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))
I've already reviewed this patch. And I am not agree with this suggestion.
We prepare the key with HFSPLUS_ROOT_CNID [1]:
err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, &str);
The hfs_brec_read() executes the search of the record [2]:
res = hfs_brec_find(fd);
if (res)
return res;
The hfs_brec_find() should found the record for requested key. And if the found
thread record contains not correct CNID, then we can check the found thread
record and return error as the result of the search.
Thanks,
Slava.
[1] https://elixir.bootlin.com/linux/v7.0-rc6/source/fs/hfsplus/super.c#L571
[2] https://elixir.bootlin.com/linux/v7.0-rc6/source/fs/hfs/bfind.c#L174
On 2026/03/31 6:45, Viacheslav Dubeyko wrote:
> I've already reviewed this patch. And I am not agree with this suggestion.
>
> We prepare the key with HFSPLUS_ROOT_CNID [1]:
>
> err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, &str);
What we are talking about is not hfsplus but hfs.
I can't catch why you are talking about hfsplus function.
>
> The hfs_brec_read() executes the search of the record [2]:
>
> res = hfs_brec_find(fd);
> if (res)
> return res;
>
> The hfs_brec_find() should found the record for requested key. And if the found
> thread record contains not correct CNID, then we can check the found thread
> record and return error as the result of the search.
hfs_brec_read() indeed calls hfs_brec_find(). But I can't interpret how to extract
CNID as of returning from hfs_brec_find().
int hfs_brec_read(struct hfs_find_data *fd, void *rec, u32 rec_len)
{
int res;
res = hfs_brec_find(fd);
if (res)
return res;
if (fd->entrylength > rec_len)
return -EINVAL;
hfs_bnode_read(fd->bnode, rec, fd->entryoffset, fd->entrylength);
return 0;
}
Since hfs_brec_read() doesn't know which type of struct (one of "struct hfs_cat_file",
"struct hfs_cat_dir" or "struct hfs_cat_thread") does the caller of hfs_brec_read()
want to read, I don't think hfs_brec_read() can tell whether the CNID is correct.
I am waiting for your response on
https://lkml.kernel.org/r/9f66743b-70e0-4886-884e-5203f5c02ed8@I-love.SAKURA.ne.jp
where the caller of hfs_brec_read() can tell whether the CNID is correct. But such
change is a matter of preference.
You can respond with your patch which will be much faster.
On Tue, 2026-03-31 at 10:12 +0900, Tetsuo Handa wrote:
> On 2026/03/31 6:45, Viacheslav Dubeyko wrote:
> > I've already reviewed this patch. And I am not agree with this suggestion.
> >
> > We prepare the key with HFSPLUS_ROOT_CNID [1]:
> >
> > err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, &str);
>
> What we are talking about is not hfsplus but hfs.
> I can't catch why you are talking about hfsplus function.
There are a lot of similarity between HFS and HFS+ b-trees functionality. Even
we can have a common code in the form of library shared between HFS and HFS+
code. HFS and HFS+ have pretty the same function names in b-tree
implementations.
>
> >
> > The hfs_brec_read() executes the search of the record [2]:
> >
> > res = hfs_brec_find(fd);
> > if (res)
> > return res;
> >
> > The hfs_brec_find() should found the record for requested key. And if the found
> > thread record contains not correct CNID, then we can check the found thread
> > record and return error as the result of the search.
>
> hfs_brec_read() indeed calls hfs_brec_find(). But I can't interpret how to extract
> CNID as of returning from hfs_brec_find().
/* The catalog record for a file */
struct hfs_cat_file {
s8 type; /* The type of entry */
u8 reserved;
u8 Flags; /* Flags such as read-only */
s8 Typ; /* file version number = 0 */
struct hfs_finfo UsrWds; /* data used by the Finder */
__be32 FlNum; /* The CNID */
__be16 StBlk; /* obsolete */
__be32 LgLen; /* The logical EOF of the data fork*/
__be32 PyLen; /* The physical EOF of the data fork */
__be16 RStBlk; /* obsolete */
__be32 RLgLen; /* The logical EOF of the rsrc fork */
__be32 RPyLen; /* The physical EOF of the rsrc fork */
__be32 CrDat; /* The creation date */
__be32 MdDat; /* The modified date */
__be32 BkDat; /* The last backup date */
struct hfs_fxinfo FndrInfo; /* more data for the Finder */
__be16 ClpSize; /* number of bytes to allocate
when extending files */
hfs_extent_rec ExtRec; /* first extent record
for the data fork */
hfs_extent_rec RExtRec; /* first extent record
for the resource fork */
u32 Resrv; /* reserved by Apple */
} __packed;
/* the catalog record for a directory */
struct hfs_cat_dir {
s8 type; /* The type of entry */
u8 reserved;
__be16 Flags; /* flags */
__be16 Val; /* Valence: number of files and
dirs in the directory */
__be32 DirID; /* The CNID */
__be32 CrDat; /* The creation date */
__be32 MdDat; /* The modification date */
__be32 BkDat; /* The last backup date */
struct hfs_dinfo UsrInfo; /* data used by the Finder */
struct hfs_dxinfo FndrInfo; /* more data used by Finder */
u8 Resrv[16]; /* reserved by Apple */
} __packed;
/* the catalog record for a thread */
struct hfs_cat_thread {
s8 type; /* The type of entry */
u8 reserved[9]; /* reserved by Apple */
__be32 ParID; /* CNID of parent directory */
struct hfs_name CName; /* The name of this entry */
} __packed;
/* A catalog tree record */
typedef union hfs_cat_rec {
s8 type; /* The type of entry */
struct hfs_cat_file file;
struct hfs_cat_dir dir;
struct hfs_cat_thread thread;
} hfs_cat_rec;
Every record starts with type. And anyone can easily retrieve the type of
record. And, then, you can extract the CNID.
>
> int hfs_brec_read(struct hfs_find_data *fd, void *rec, u32 rec_len)
> {
> int res;
>
> res = hfs_brec_find(fd);
> if (res)
> return res;
> if (fd->entrylength > rec_len)
> return -EINVAL;
> hfs_bnode_read(fd->bnode, rec, fd->entryoffset, fd->entrylength);
> return 0;
> }
>
> Since hfs_brec_read() doesn't know which type of struct (one of "struct hfs_cat_file",
> "struct hfs_cat_dir" or "struct hfs_cat_thread") does the caller of hfs_brec_read()
> want to read, I don't think hfs_brec_read() can tell whether the CNID is correct.
Please, check the HFS's on-disk layout.
>
> I am waiting for your response on
> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_9f66743b-2D70e0-2D4886-2D884e-2D5203f5c02ed8-40I-2Dlove.SAKURA.ne.jp&d=DwICaQ&c=BSDicqBQBDjDI9RkVyTcHQ&r=q5bIm4AXMzc8NJu1_RGmnQ2fMWKq4Y4RAkElvUgSs00&m=V9I6gMC1If2D6irQzVVC1M0wKQW-kbErffC9npM7z-fyjkutPS0YOARfqxRsyUdc&s=MAn71bGJ4-F3OEEhjru0azzQc62bUcvfCsk1VndcjC4&e=
> where the caller of hfs_brec_read() can tell whether the CNID is correct. But such
> change is a matter of preference.
>
> You can respond with your patch which will be much faster.
>
Sorry, I am busy with other HFS/HFS+ issues.
Thanks,
Slava.
On Sat, Mar 28, 2026 at 04:12:07PM +0900, Tetsuo Handa wrote: > George Anthony Vernon is trying to fix this bug without my patch > ( https://lkml.kernel.org/r/20260311211309.27856-1-contact@gvernon.com ). > But it was proven that George's patch cannot fix this bug. Therefore, > I believe that we came to a conclusion that applying my patch is > the way to go. We can apply George's patch as a further improvement. > But unfortunately Viacheslav Dubeyko (the maintainer of HFS) is not > responding. Therefore, I'm sending this patch to more developers who > might accept this patch, instead of waiting for Viacheslav. Thanks for the CC. Slava (Viacheslav) got in contact with me directly on Tuesday 24th to say he missed some emails and acknowledged not being completely correct about the thread record logic which we were talking about. He indicated he would take a look at the next patch version when he received it. I will follow this thread with interest. In the meantime I will follow up my patch with a new version, thanks for your review of it. Your patch validates the superblock lookup, however my patch which you linked validates reserved CNIDs retrieved from a bad cat record on disk, which is also a reachable logical path which hits the BUG() on writeback. I can consistently reproduce that BUG() with your patch applied. Both patches are therefore necessary. Hope this helps, George
© 2016 - 2026 Red Hat, Inc.