[PATCH] hfsplus: don't use BUG_ON() in hfsplus_create_attributes_file()

[PATCH] hfsplus: don't use BUG_ON() in hfsplus_create_attributes_file()

[Tetsuo Handa]

syzkaller can mount crafted filesystem images.
Don't crash the kernel when we can continue.

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

diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
index 9a1a93e3888b..191767d4cf78 100644
--- a/fs/hfsplus/xattr.c
+++ b/fs/hfsplus/xattr.c
@@ -172,7 +172,11 @@ static int hfsplus_create_attributes_file(struct super_block *sb)
 		return PTR_ERR(attr_file);
 	}
 
-	BUG_ON(i_size_read(attr_file) != 0);
+	if (i_size_read(attr_file) != 0) {
+		err = -EIO;
+		pr_err("failed to load attributes file\n");
+		goto end_attr_file_creation;
+	}
 
 	hip = HFSPLUS_I(attr_file);
 
-- 
2.49.0

Re: [PATCH] hfsplus: don't use BUG_ON() in hfsplus_create_attributes_file()

[Viacheslav Dubeyko]

On Wed, 2025-06-25 at 19:10 +0900, Tetsuo Handa wrote:
> syzkaller can mount crafted filesystem images.
> Don't crash the kernel when we can continue.
> 
> Reported-by: syzbot <syzbot+1107451c16b9eb9d29e6@syzkaller.appspotmail.com>
> Closes: https://syzkaller.appspot.com/bug?extid=1107451c16b9eb9d29e6  
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  fs/hfsplus/xattr.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
> index 9a1a93e3888b..191767d4cf78 100644
> --- a/fs/hfsplus/xattr.c
> +++ b/fs/hfsplus/xattr.c
> @@ -172,7 +172,11 @@ static int hfsplus_create_attributes_file(struct super_block *sb)
>  		return PTR_ERR(attr_file);
>  	}
>  
> -	BUG_ON(i_size_read(attr_file) != 0);

So, it's something like unexpected situation here. Why do we have
i_size_read(attr_file) != 0 here? It looks like hfsplus_create_attributes_file()
was called in incorrect context. Probably, it's not the whole fix. Any ideas?

> +	if (i_size_read(attr_file) != 0) {
> +		err = -EIO;
> +		pr_err("failed to load attributes file\n");
> +		goto end_attr_file_creation;
> +	}
>  
>  	hip = HFSPLUS_I(attr_file);
>  

Makes sense to me. Looks good.

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

Thanks,
Slava.

Re: [PATCH] hfsplus: don't use BUG_ON() in hfsplus_create_attributes_file()

[Yangtao Li]

在 2025/7/1 01:18, Viacheslav Dubeyko 写道:
> On Wed, 2025-06-25 at 19:10 +0900, Tetsuo Handa wrote:
>> syzkaller can mount crafted filesystem images.
>> Don't crash the kernel when we can continue.
>>
>> Reported-by: syzbot <syzbot+1107451c16b9eb9d29e6@syzkaller.appspotmail.com>
>> Closes: https://syzkaller.appspot.com/bug?extid=1107451c16b9eb9d29e6
>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>> ---
>>   fs/hfsplus/xattr.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
>> index 9a1a93e3888b..191767d4cf78 100644
>> --- a/fs/hfsplus/xattr.c
>> +++ b/fs/hfsplus/xattr.c
>> @@ -172,7 +172,11 @@ static int hfsplus_create_attributes_file(struct super_block *sb)
>>   		return PTR_ERR(attr_file);
>>   	}
>>   
>> -	BUG_ON(i_size_read(attr_file) != 0);
> 
> So, it's something like unexpected situation here. Why do we have
> i_size_read(attr_file) != 0 here? It looks like hfsplus_create_attributes_file()
> was called in incorrect context. Probably, it's not the whole fix. Any ideas?

161         case HFSPLUS_VALID_ATTR_TREE:
162                 return 0;
163         case HFSPLUS_FAILED_ATTR_TREE:
164                 return -EOPNOTSUPP;
165         default:
166                 BUG();
167         }

I haven't delved into the implementation details of xattr yet, but there 
is a bug in this function. It seems that we should convert the bug to 
return EIO in another patch?

Otherwise LGTM.

Reviewed-by: Yangtao Li <frank.li@vivo.com>

Thx,
Yangtao

Re: [PATCH] hfsplus: don't use BUG_ON() in hfsplus_create_attributes_file()

[Tetsuo Handa]

On 2025/07/07 23:22, Yangtao Li wrote:
> 161         case HFSPLUS_VALID_ATTR_TREE:
> 162                 return 0;
> 163         case HFSPLUS_FAILED_ATTR_TREE:
> 164                 return -EOPNOTSUPP;
> 165         default:
> 166                 BUG();
> 167         }
> 
> I haven't delved into the implementation details of xattr yet, but
> there is a bug in this function. It seems that we should convert
> the bug to return EIO in another patch?

I don't think this BUG() is triggerable. attr_tree_state is an atomic_t
which can take only one of HFSPLUS_EMPTY_ATTR_TREE, HFSPLUS_VALID_ATTR_TREE,
HFSPLUS_FAILED_ATTR_TREE or HFSPLUS_CREATING_ATTR_TREE.

RE: [PATCH] hfsplus: don't use BUG_ON() in hfsplus_create_attributes_file()

[Viacheslav Dubeyko]

On Mon, 2025-07-07 at 23:45 +0900, Tetsuo Handa wrote:
> On 2025/07/07 23:22, Yangtao Li wrote:
> > 161         case HFSPLUS_VALID_ATTR_TREE:
> > 162                 return 0;
> > 163         case HFSPLUS_FAILED_ATTR_TREE:
> > 164                 return -EOPNOTSUPP;
> > 165         default:
> > 166                 BUG();
> > 167         }
> > 
> > I haven't delved into the implementation details of xattr yet, but
> > there is a bug in this function. It seems that we should convert
> > the bug to return EIO in another patch?
> 
> I don't think this BUG() is triggerable. attr_tree_state is an atomic_t
> which can take only one of HFSPLUS_EMPTY_ATTR_TREE, HFSPLUS_VALID_ATTR_TREE,
> HFSPLUS_FAILED_ATTR_TREE or HFSPLUS_CREATING_ATTR_TREE.

It's completely correct conclusion. The goal of this BUG() simply to trigger the
crash if somebody will change the set of possible states of attr_tree_state. But
this logic will not be reworked.

>> @@ -172,7 +172,11 @@ static int hfsplus_create_attributes_file(struct
super_block *sb)
>>   		return PTR_ERR(attr_file);
>>   	}
>>  
>> -	BUG_ON(i_size_read(attr_file) != 0);

But I still worry about i_size_read(attr_file). How this size could be not zero
during hfsplus_create_attributes_file() call?

Thanks,
Slava.

Re: [PATCH] hfsplus: don't use BUG_ON() in hfsplus_create_attributes_file()

[Tetsuo Handa]

On 2025/07/08 4:03, Viacheslav Dubeyko wrote:
>>> @@ -172,7 +172,11 @@ static int hfsplus_create_attributes_file(struct
> super_block *sb)
>>>   		return PTR_ERR(attr_file);
>>>   	}
>>>  
>>> -	BUG_ON(i_size_read(attr_file) != 0);
> 
> But I still worry about i_size_read(attr_file). How this size could be not zero
> during hfsplus_create_attributes_file() call?

Because the filesystem image is intentionally crafted.

syzkaller mounts this image which already contains inode for xattr file
but vhdr->attr_file.total_blocks at
https://elixir.bootlin.com/linux/v6.16-rc5/source/fs/hfsplus/super.c#L485
is 0. This inconsistency is not detected during mount operation, and
sbi->attr_tree_state remains HFSPLUS_EMPTY_ATTR_TREE, and
this inconsistency is detected when setxattr operation is called.

The correct fix might be to implement stricter consistency check during
mount operation, but even userspace fsck.hfsplus is not doing such check.

RE: [PATCH] hfsplus: don't use BUG_ON() in hfsplus_create_attributes_file()

[Viacheslav Dubeyko]

On Wed, 2025-07-09 at 23:02 +0900, Tetsuo Handa wrote:
> On 2025/07/08 4:03, Viacheslav Dubeyko wrote:
> > > > @@ -172,7 +172,11 @@ static int hfsplus_create_attributes_file(struct
> > super_block *sb)
> > > >   		return PTR_ERR(attr_file);
> > > >   	}
> > > >  
> > > > -	BUG_ON(i_size_read(attr_file) != 0);
> > 
> > But I still worry about i_size_read(attr_file). How this size could be not zero
> > during hfsplus_create_attributes_file() call?
> 
> Because the filesystem image is intentionally crafted.
> 
> syzkaller mounts this image which already contains inode for xattr file
> but vhdr->attr_file.total_blocks at
> https://elixir.bootlin.com/linux/v6.16-rc5/source/fs/hfsplus/super.c#L485  
> is 0. This inconsistency is not detected during mount operation, and
> sbi->attr_tree_state remains HFSPLUS_EMPTY_ATTR_TREE, and
> this inconsistency is detected when setxattr operation is called.
> 
> The correct fix might be to implement stricter consistency check during
> mount operation, but even userspace fsck.hfsplus is not doing such check.

As far as I can see, we try to create Attributes File in __hfsplus_setxattr()
because the mount logic doesn't create this file (because it could not exists or
not necessary):

int __hfsplus_setxattr(struct inode *inode, const char *name,
			const void *value, size_t size, int flags)
{
<skipped>

	if (!HFSPLUS_SB(inode->i_sb)->attr_tree) {
		err = hfsplus_create_attributes_file(inode->i_sb);
		if (unlikely(err))
			goto end_setxattr;
	}

<skipped>
}
My worry that we could have a race condition here. Let's imagine that two
threads are trying to call __hfsplus_setxattr() and both will try to create the
Attributes File. Potentially, we could end in situation when inode could have
not zero size during hfsplus_create_attributes_file() in one thread because
another thread in the middle of Attributes File creation. Could we double check
that we don't have the race condition here? Otherwise, we need to make much
cleaner fix of this issue.

Thanks,
Slava.

Re: [PATCH] hfsplus: don't use BUG_ON() in hfsplus_create_attributes_file()

[Tetsuo Handa]

On 2025/07/10 3:33, Viacheslav Dubeyko wrote:
> My worry that we could have a race condition here. Let's imagine that two
> threads are trying to call __hfsplus_setxattr() and both will try to create the
> Attributes File. Potentially, we could end in situation when inode could have
> not zero size during hfsplus_create_attributes_file() in one thread because
> another thread in the middle of Attributes File creation. Could we double check
> that we don't have the race condition here? Otherwise, we need to make much
> cleaner fix of this issue.

I think that there is some sort of race window, for
https://elixir.bootlin.com/linux/v6.15.5/source/fs/hfsplus/xattr.c#L145
explains that if more than one thread concurrently reached

	if (!HFSPLUS_SB(inode->i_sb)->attr_tree) {
		err = hfsplus_create_attributes_file(inode->i_sb);
		if (unlikely(err))
			goto end_setxattr;
	}

path, all threads except one thread will fail with -EAGAIN.

Re: [PATCH] hfsplus: don't use BUG_ON() in hfsplus_create_attributes_file()

[Tetsuo Handa]

On 2025/07/10 7:03, Tetsuo Handa wrote:
> On 2025/07/10 3:33, Viacheslav Dubeyko wrote:
>> My worry that we could have a race condition here. Let's imagine that two
>> threads are trying to call __hfsplus_setxattr() and both will try to create the
>> Attributes File. Potentially, we could end in situation when inode could have
>> not zero size during hfsplus_create_attributes_file() in one thread because
>> another thread in the middle of Attributes File creation. Could we double check
>> that we don't have the race condition here? Otherwise, we need to make much
>> cleaner fix of this issue.
> 
> I think that there is some sort of race window, for
> https://elixir.bootlin.com/linux/v6.15.5/source/fs/hfsplus/xattr.c#L145
> explains that if more than one thread concurrently reached
> 
> 	if (!HFSPLUS_SB(inode->i_sb)->attr_tree) {
> 		err = hfsplus_create_attributes_file(inode->i_sb);
> 		if (unlikely(err))
> 			goto end_setxattr;
> 	}
> 
> path, all threads except one thread will fail with -EAGAIN.
> 

Do you prefer stricter mount-time validation shown below?
Is vhdr->attr_file.total_blocks == 0 when sbi->attr_tree exists and is empty?

diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index 948b8aaee33e..f6324a0458f3 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -482,13 +482,17 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
 		goto out_close_ext_tree;
 	}
 	atomic_set(&sbi->attr_tree_state, HFSPLUS_EMPTY_ATTR_TREE);
-	if (vhdr->attr_file.total_blocks != 0) {
-		sbi->attr_tree = hfs_btree_open(sb, HFSPLUS_ATTR_CNID);
-		if (!sbi->attr_tree) {
-			pr_err("failed to load attributes file\n");
-			goto out_close_cat_tree;
+	sbi->attr_tree = hfs_btree_open(sb, HFSPLUS_ATTR_CNID);
+	if (sbi->attr_tree) {
+		if (vhdr->attr_file.total_blocks != 0) {
+			atomic_set(&sbi->attr_tree_state, HFSPLUS_VALID_ATTR_TREE);
+		} else {
+			pr_err("found attributes file despite total blocks is 0\n");
+			goto out_close_attr_tree;
 		}
-		atomic_set(&sbi->attr_tree_state, HFSPLUS_VALID_ATTR_TREE);
+	} else if (vhdr->attr_file.total_blocks != 0) {
+		pr_err("failed to load attributes file\n");
+		goto out_close_cat_tree;
 	}
 	sb->s_xattr = hfsplus_xattr_handlers;
 

RE: [PATCH] hfsplus: don't use BUG_ON() in hfsplus_create_attributes_file()

[Viacheslav Dubeyko]

On Fri, 2025-07-11 at 20:35 +0900, Tetsuo Handa wrote:
> On 2025/07/10 7:03, Tetsuo Handa wrote:
> > On 2025/07/10 3:33, Viacheslav Dubeyko wrote:
> > > My worry that we could have a race condition here. Let's imagine that two
> > > threads are trying to call __hfsplus_setxattr() and both will try to create the
> > > Attributes File. Potentially, we could end in situation when inode could have
> > > not zero size during hfsplus_create_attributes_file() in one thread because
> > > another thread in the middle of Attributes File creation. Could we double check
> > > that we don't have the race condition here? Otherwise, we need to make much
> > > cleaner fix of this issue.
> > 
> > I think that there is some sort of race window, for
> > https://elixir.bootlin.com/linux/v6.15.5/source/fs/hfsplus/xattr.c#L145  
> > explains that if more than one thread concurrently reached
> > 
> > 	if (!HFSPLUS_SB(inode->i_sb)->attr_tree) {
> > 		err = hfsplus_create_attributes_file(inode->i_sb);
> > 		if (unlikely(err))
> > 			goto end_setxattr;
> > 	}
> > 
> > path, all threads except one thread will fail with -EAGAIN.
> > 
> 
> Do you prefer stricter mount-time validation shown below?
> Is vhdr->attr_file.total_blocks == 0 when sbi->attr_tree exists and is empty?
> 
> diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
> index 948b8aaee33e..f6324a0458f3 100644
> --- a/fs/hfsplus/super.c
> +++ b/fs/hfsplus/super.c
> @@ -482,13 +482,17 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
>  		goto out_close_ext_tree;
>  	}
>  	atomic_set(&sbi->attr_tree_state, HFSPLUS_EMPTY_ATTR_TREE);
> -	if (vhdr->attr_file.total_blocks != 0) {
> -		sbi->attr_tree = hfs_btree_open(sb, HFSPLUS_ATTR_CNID);
> -		if (!sbi->attr_tree) {
> -			pr_err("failed to load attributes file\n");
> -			goto out_close_cat_tree;
> +	sbi->attr_tree = hfs_btree_open(sb, HFSPLUS_ATTR_CNID);
> +	if (sbi->attr_tree) {
> +		if (vhdr->attr_file.total_blocks != 0) {
> +			atomic_set(&sbi->attr_tree_state, HFSPLUS_VALID_ATTR_TREE);
> +		} else {
> +			pr_err("found attributes file despite total blocks is 0\n");
> +			goto out_close_attr_tree;
>  		}
> -		atomic_set(&sbi->attr_tree_state, HFSPLUS_VALID_ATTR_TREE);
> +	} else if (vhdr->attr_file.total_blocks != 0) {
> +		pr_err("failed to load attributes file\n");
> +		goto out_close_cat_tree;
>  	}
>  	sb->s_xattr = hfsplus_xattr_handlers;
>  

Frankly speaking, I still don't see the whole picture here. If we have created
the Attribute File during mount operation, then why should we try to create the
Attributes File during __hfsplus_setxattr() call? If we didn't create the
Attributes File during the mount time and HFSPLUS_SB(inode->i_sb)->attr_tree is
NULL, then how i_size_read(attr_file) != 0? Even if we are checking vhdr-
>attr_file.total_blocks, then it doesn't provide guarantee that
i_size_read(attr_file) is zero too. Something is wrong in this situation and
more stricter mount time validation cannot guarantee against the situation that
you are trying to solve in the issue. We are missing something here.

Thanks,
Slava.

Re: [PATCH] hfsplus: don't use BUG_ON() in hfsplus_create_attributes_file()

[Tetsuo Handa]

On 2025/07/12 2:21, Viacheslav Dubeyko wrote:
> Frankly speaking, I still don't see the whole picture here. If we have created
> the Attribute File during mount operation, then why should we try to create the
> Attributes File during __hfsplus_setxattr() call? If we didn't create the
> Attributes File during the mount time and HFSPLUS_SB(inode->i_sb)->attr_tree is
> NULL, then how i_size_read(attr_file) != 0? Even if we are checking vhdr-
>> attr_file.total_blocks, then it doesn't provide guarantee that
> i_size_read(attr_file) is zero too. Something is wrong in this situation and
> more stricter mount time validation cannot guarantee against the situation that
> you are trying to solve in the issue. We are missing something here.

I still don't see what you are missing.

When hfsplus_iget(sb, HFSPLUS_ATTR_CNID) is called from hfsplus_create_attributes_file(sb),
hfsplus_system_read_inode(inode) from hfsplus_iget(HFSPLUS_ATTR_CNID) calls
hfsplus_inode_read_fork(inode, &vhdr->attr_file). Since hfsplus_inode_read_fork() calls
inode_set_bytes(), it is natural that i_size_read(attr_file) != 0 when returning from
hfsplus_iget(sb, HFSPLUS_ATTR_CNID).

At this point, the only question should be why hfsplus_inode_read_fork() from
hfsplus_system_read_inode(inode) from hfsplus_iget() is not called from hfsplus_fill_super()
when the Attributes File already exists and its size is not 0. And the reason is that
hfsplus_iget(sb, HFSPLUS_ATTR_CNID) from hfs_btree_open(sb, HFSPLUS_ATTR_CNID) is called
only when vhdr->attr_file.total_blocks != 0.

That is, when "vhdr" contains erroneous values (in the reproducer, vhdr->attr_file.total_blocks
is 0) that do not reflect the actual state of the filesystem (in the reproducer, inode_set_bytes()
sets non-zero value despite vhdr->attr_file.total_blocks is 0), hfsplus_fill_super() fails to call
hfs_btree_open(sb, HFSPLUS_ATTR_CNID) at mount time.

You can easily reproduce this problem by compiling and running the reproducer
at https://syzkaller.appspot.com/text?tag=ReproC&x=15f6b9d4580000 after you run
"losetup -f" which creates /dev/loop0 needed by the reproducer.

I noticed that the reason fsck.hfsplus could not detect errors is that the filesystem
image in the reproducer was compressed. If I run fsck.hfsplus on uncompressed image,
fsck.hfsplus generated the following messages.

# fsck.hfsplus hfsplus.img
** hfsplus.img
   Executing fsck_hfs (version 540.1-Linux).
** Checking non-journaled HFS Plus Volume.
   The volume name is untitled
** Checking extents overflow file.
** Checking catalog file.
   Invalid extent entry
(4, 1)
** Checking multi-linked files.
** Checking catalog hierarchy.
** Checking extended attributes file.
** Checking volume bitmap.
** Checking volume information.
** Repairing volume.
   Look for links to corrupt files in DamagedFiles directory.
** Rechecking volume.
** Checking non-journaled HFS Plus Volume.
   The volume name is untitled
** Checking extents overflow file.
** Checking catalog file.
** Checking multi-linked files.
** Checking catalog hierarchy.
** Checking extended attributes file.
** Checking volume bitmap.
   Volume bitmap needs minor repair for under-allocation
** Checking volume information.
   Invalid volume free block count
   (It should be 179 instead of 180)
** Repairing volume.
** Rechecking volume.
** Checking non-journaled HFS Plus Volume.
   The volume name is untitled
** Checking extents overflow file.
** Checking catalog file.
** Checking multi-linked files.
** Checking catalog hierarchy.
** Checking extended attributes file.
** Checking volume bitmap.
** Checking volume information.
** The volume untitled was repaired successfully.

RE: [PATCH] hfsplus: don't use BUG_ON() in hfsplus_create_attributes_file()

[Viacheslav Dubeyko]

On Sat, 2025-07-12 at 20:22 +0900, Tetsuo Handa wrote:
> On 2025/07/12 2:21, Viacheslav Dubeyko wrote:
> > Frankly speaking, I still don't see the whole picture here. If we have created
> > the Attribute File during mount operation, then why should we try to create the
> > Attributes File during __hfsplus_setxattr() call? If we didn't create the
> > Attributes File during the mount time and HFSPLUS_SB(inode->i_sb)->attr_tree is
> > NULL, then how i_size_read(attr_file) != 0? Even if we are checking vhdr-
> > > attr_file.total_blocks, then it doesn't provide guarantee that
> > i_size_read(attr_file) is zero too. Something is wrong in this situation and
> > more stricter mount time validation cannot guarantee against the situation that
> > you are trying to solve in the issue. We are missing something here.
> 
> I still don't see what you are missing.
> 
> When hfsplus_iget(sb, HFSPLUS_ATTR_CNID) is called from hfsplus_create_attributes_file(sb),
> hfsplus_system_read_inode(inode) from hfsplus_iget(HFSPLUS_ATTR_CNID) calls
> hfsplus_inode_read_fork(inode, &vhdr->attr_file). Since hfsplus_inode_read_fork() calls
> inode_set_bytes(), it is natural that i_size_read(attr_file) != 0 when returning from
> hfsplus_iget(sb, HFSPLUS_ATTR_CNID).
> 
> At this point, the only question should be why hfsplus_inode_read_fork() from
> hfsplus_system_read_inode(inode) from hfsplus_iget() is not called from hfsplus_fill_super()
> when the Attributes File already exists and its size is not 0. And the reason is that
> hfsplus_iget(sb, HFSPLUS_ATTR_CNID) from hfs_btree_open(sb, HFSPLUS_ATTR_CNID) is called
> only when vhdr->attr_file.total_blocks != 0.
> 
> That is, when "vhdr" contains erroneous values (in the reproducer, vhdr->attr_file.total_blocks
> is 0) that do not reflect the actual state of the filesystem (in the reproducer, inode_set_bytes()
> sets non-zero value despite vhdr->attr_file.total_blocks is 0), hfsplus_fill_super() fails to call
> hfs_btree_open(sb, HFSPLUS_ATTR_CNID) at mount time.
> 

Yes, you are right here. I was missing that we have corrupted state of the
volume.

Related to reworking the mount-time validation logic... I think it's not very
good idea because hfs_btree_open() will:
(1) allocate memory: tree = kzalloc(sizeof(*tree), GFP_KERNEL);
(2) get inode: inode = hfsplus_iget(sb, id);
(3) try to read memory page: page = read_mapping_page(mapping, 0, NULL);
And, then, we need to free memory and to make all necessary cleanup. It's too
much activity for the really empty tree.

But we can make some improvement of the initial patch. We definitely know that
it's the volume corruption. So, from my point of view, it makes sense to add
comment that explains the whole situation and change the pr_err() message with
recommendation to run the FSCK tool:

if (i_size_read(attr_file) != 0) {
    err = -EIO;
    /* explain here how we detected that volume is corrupted */
    pr_err("HFS+ superblock contains incorrect Attributes File details. "
           "Probably, volume is corrupted!!! Please, run FSCK tool!!!\n");
    goto end_attr_file_creation;
}

You can modify my comments as you like. :)

Thanks,
Slava.

> You can easily reproduce this problem by compiling and running the reproducer
> at https://syzkaller.appspot.com/text?tag=ReproC&x=15f6b9d4580000   after you run
> "losetup -f" which creates /dev/loop0 needed by the reproducer.
> 
> I noticed that the reason fsck.hfsplus could not detect errors is that the filesystem
> image in the reproducer was compressed. If I run fsck.hfsplus on uncompressed image,
> fsck.hfsplus generated the following messages.
> 
> # fsck.hfsplus hfsplus.img
> ** hfsplus.img
>    Executing fsck_hfs (version 540.1-Linux).
> ** Checking non-journaled HFS Plus Volume.
>    The volume name is untitled
> ** Checking extents overflow file.
> ** Checking catalog file.
>    Invalid extent entry
> (4, 1)
> ** Checking multi-linked files.
> ** Checking catalog hierarchy.
> ** Checking extended attributes file.
> ** Checking volume bitmap.
> ** Checking volume information.
> ** Repairing volume.
>    Look for links to corrupt files in DamagedFiles directory.
> ** Rechecking volume.
> ** Checking non-journaled HFS Plus Volume.
>    The volume name is untitled
> ** Checking extents overflow file.
> ** Checking catalog file.
> ** Checking multi-linked files.
> ** Checking catalog hierarchy.
> ** Checking extended attributes file.
> ** Checking volume bitmap.
>    Volume bitmap needs minor repair for under-allocation
> ** Checking volume information.
>    Invalid volume free block count
>    (It should be 179 instead of 180)
> ** Repairing volume.
> ** Rechecking volume.
> ** Checking non-journaled HFS Plus Volume.
>    The volume name is untitled
> ** Checking extents overflow file.
> ** Checking catalog file.
> ** Checking multi-linked files.
> ** Checking catalog hierarchy.
> ** Checking extended attributes file.
> ** Checking volume bitmap.
> ** Checking volume information.
> ** The volume untitled was repaired successfully.

[PATCH v2] hfsplus: don't use BUG_ON() in hfsplus_create_attributes_file()

[Tetsuo Handa]

When the volume header contains erroneous values that do not reflect
the actual state of the filesystem, hfsplus_fill_super() assumes that
the attributes file is not yet created, which later results in hitting
BUG_ON() when hfsplus_create_attributes_file() is called. Replace this
BUG_ON() with -EIO error with a message to suggest running fsck tool.

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

diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
index 9a1a93e3888b..18dc3d254d21 100644
--- a/fs/hfsplus/xattr.c
+++ b/fs/hfsplus/xattr.c
@@ -172,7 +172,11 @@ static int hfsplus_create_attributes_file(struct super_block *sb)
 		return PTR_ERR(attr_file);
 	}
 
-	BUG_ON(i_size_read(attr_file) != 0);
+	if (i_size_read(attr_file) != 0) {
+		err = -EIO;
+		pr_err("detected inconsistent attributes file, running fsck.hfsplus is recommended.\n");
+		goto end_attr_file_creation;
+	}
 
 	hip = HFSPLUS_I(attr_file);
 
-- 
2.50.1

Re: [PATCH v2] hfsplus: don't use BUG_ON() in hfsplus_create_attributes_file()

[Viacheslav Dubeyko]

On Tue, 2025-07-15 at 14:17 +0900, Tetsuo Handa wrote:
> When the volume header contains erroneous values that do not reflect
> the actual state of the filesystem, hfsplus_fill_super() assumes that
> the attributes file is not yet created, which later results in hitting
> BUG_ON() when hfsplus_create_attributes_file() is called. Replace this
> BUG_ON() with -EIO error with a message to suggest running fsck tool.
> 
> Reported-by: syzbot <syzbot+1107451c16b9eb9d29e6@syzkaller.appspotmail.com>
> Closes: https://syzkaller.appspot.com/bug?extid=1107451c16b9eb9d29e6  
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  fs/hfsplus/xattr.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
> index 9a1a93e3888b..18dc3d254d21 100644
> --- a/fs/hfsplus/xattr.c
> +++ b/fs/hfsplus/xattr.c
> @@ -172,7 +172,11 @@ static int hfsplus_create_attributes_file(struct super_block *sb)
>  		return PTR_ERR(attr_file);
>  	}
>  
> -	BUG_ON(i_size_read(attr_file) != 0);
> +	if (i_size_read(attr_file) != 0) {
> +		err = -EIO;
> +		pr_err("detected inconsistent attributes file, running fsck.hfsplus is recommended.\n");
> +		goto end_attr_file_creation;
> +	}
>  
>  	hip = HFSPLUS_I(attr_file);
>  

Looks good!

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

Thanks,
Slava.