hfsplus: set attributes inode dirty at correct position

[PATCH] hfsplus: set attributes inode dirty at correct position

Posted by Edward Adam Davis 2 months ago

Syzbot reported a null-ptr-deref in [1].
If the attributes file is not loaded during system mount, a trigger
occurs [1] when setxattr is executed in userspace.

Move the mark inode dirty operation to a point after the attr_tree has
been successfully acquired.

[1]
KASAN: null-ptr-deref in range [0x0000000000000008-0x000000000000000f]
Call Trace:
 hfsplus_setxattr+0x124/0x340 fs/hfsplus/xattr.c:555
 hfsplus_trusted_setxattr+0x40/0x60 fs/hfsplus/xattr_trusted.c:30
 __vfs_setxattr+0x43c/0x480 fs/xattr.c:218
 __vfs_setxattr_noperm+0x12d/0x660 fs/xattr.c:252
 vfs_setxattr+0x163/0x360 fs/xattr.c:339
 do_setxattr fs/xattr.c:654 [inline]

Reported-by: syzbot+bc70a12e438dadba4fb4@syzkaller.appspotmail.com
Fixes: ee8422d00b7c ("hfsplus: fix potential Allocation File corruption after fsync")
Closes: https://syzkaller.appspot.com/bug?extid=bc70a12e438dadba4fb4
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
 fs/hfsplus/xattr.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
index 452a1f9becb2..3e6f45b3259d 100644
--- a/fs/hfsplus/xattr.c
+++ b/fs/hfsplus/xattr.c
@@ -317,12 +317,14 @@ static int hfsplus_create_attributes_file(struct super_block *sb)
 		next_node++;
 	}
 
-	hfsplus_mark_inode_dirty(HFSPLUS_ATTR_TREE_I(sb), HFSPLUS_I_ATTR_DIRTY);
 	hfsplus_mark_inode_dirty(attr_file, HFSPLUS_I_ATTR_DIRTY);
 
 	sbi->attr_tree = hfs_btree_open(sb, HFSPLUS_ATTR_CNID);
 	if (!sbi->attr_tree)
 		pr_err("failed to load attributes file\n");
+	else
+		hfsplus_mark_inode_dirty(HFSPLUS_ATTR_TREE_I(sb),
+					 HFSPLUS_I_ATTR_DIRTY);
 
 failed_header_node_init:
 	kfree(buf);
-- 
2.43.0

Re: [PATCH] hfsplus: set attributes inode dirty at correct position

Posted by Viacheslav Dubeyko 2 months ago

On Wed, 2026-04-15 at 16:45 +0800, Edward Adam Davis wrote:
> Syzbot reported a null-ptr-deref in [1].
> If the attributes file is not loaded during system mount, a trigger
> occurs [1] when setxattr is executed in userspace.
> 
> Move the mark inode dirty operation to a point after the attr_tree has
> been successfully acquired.
> 
> [1]
> KASAN: null-ptr-deref in range [0x0000000000000008-0x000000000000000f]
> Call Trace:
>  hfsplus_setxattr+0x124/0x340 fs/hfsplus/xattr.c:555
>  hfsplus_trusted_setxattr+0x40/0x60 fs/hfsplus/xattr_trusted.c:30
>  __vfs_setxattr+0x43c/0x480 fs/xattr.c:218
>  __vfs_setxattr_noperm+0x12d/0x660 fs/xattr.c:252
>  vfs_setxattr+0x163/0x360 fs/xattr.c:339
>  do_setxattr fs/xattr.c:654 [inline]
> 
> Reported-by: syzbot+bc70a12e438dadba4fb4@syzkaller.appspotmail.com
> Fixes: ee8422d00b7c ("hfsplus: fix potential Allocation File corruption after fsync")
> Closes: https://syzkaller.appspot.com/bug?extid=bc70a12e438dadba4fb4
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> ---
>  fs/hfsplus/xattr.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
> index 452a1f9becb2..3e6f45b3259d 100644
> --- a/fs/hfsplus/xattr.c
> +++ b/fs/hfsplus/xattr.c
> @@ -317,12 +317,14 @@ static int hfsplus_create_attributes_file(struct super_block *sb)
>  		next_node++;
>  	}
>  
> -	hfsplus_mark_inode_dirty(HFSPLUS_ATTR_TREE_I(sb), HFSPLUS_I_ATTR_DIRTY);

It's really strange that xfstests didn't catch the issue. Probably, we need to
have the specialized HFS+ tests.

>  	hfsplus_mark_inode_dirty(attr_file, HFSPLUS_I_ATTR_DIRTY);
>  
>  	sbi->attr_tree = hfs_btree_open(sb, HFSPLUS_ATTR_CNID);
>  	if (!sbi->attr_tree)
>  		pr_err("failed to load attributes file\n");
> +	else
> +		hfsplus_mark_inode_dirty(HFSPLUS_ATTR_TREE_I(sb),
> +					 HFSPLUS_I_ATTR_DIRTY);

As far as I can see, HFSPLUS_ATTR_TREE_I(sb) and attr_file are the same
entities. Am I right here? :) So, we can simply remove the first
hfsplus_mark_inode_dirty(). Does it make sense?

Thanks,
Slava.

>  
>  failed_header_node_init:
>  	kfree(buf);

Re: [PATCH] hfsplus: set attributes inode dirty at correct position

Posted by Edward Adam Davis 2 months ago

On Wed, 15 Apr 2026 15:11:13 -0700, Viacheslav Dubeyko wrote:
> > Syzbot reported a null-ptr-deref in [1].
> > If the attributes file is not loaded during system mount, a trigger
> > occurs [1] when setxattr is executed in userspace.
> >
> > Move the mark inode dirty operation to a point after the attr_tree has
> > been successfully acquired.
> >
> > [1]
> > KASAN: null-ptr-deref in range [0x0000000000000008-0x000000000000000f]
> > Call Trace:
> >  hfsplus_setxattr+0x124/0x340 fs/hfsplus/xattr.c:555
> >  hfsplus_trusted_setxattr+0x40/0x60 fs/hfsplus/xattr_trusted.c:30
> >  __vfs_setxattr+0x43c/0x480 fs/xattr.c:218
> >  __vfs_setxattr_noperm+0x12d/0x660 fs/xattr.c:252
> >  vfs_setxattr+0x163/0x360 fs/xattr.c:339
> >  do_setxattr fs/xattr.c:654 [inline]
> >
> > Reported-by: syzbot+bc70a12e438dadba4fb4@syzkaller.appspotmail.com
> > Fixes: ee8422d00b7c ("hfsplus: fix potential Allocation File corruption after fsync")
> > Closes: https://syzkaller.appspot.com/bug?extid=bc70a12e438dadba4fb4
> > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > ---
> >  fs/hfsplus/xattr.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
> > index 452a1f9becb2..3e6f45b3259d 100644
> > --- a/fs/hfsplus/xattr.c
> > +++ b/fs/hfsplus/xattr.c
> > @@ -317,12 +317,14 @@ static int hfsplus_create_attributes_file(struct super_block *sb)
> >  		next_node++;
> >  	}
> >
> > -	hfsplus_mark_inode_dirty(HFSPLUS_ATTR_TREE_I(sb), HFSPLUS_I_ATTR_DIRTY);
> 
> It's really strange that xfstests didn't catch the issue. Probably, we need to
> have the specialized HFS+ tests.
> 
> >  	hfsplus_mark_inode_dirty(attr_file, HFSPLUS_I_ATTR_DIRTY);
> >
> >  	sbi->attr_tree = hfs_btree_open(sb, HFSPLUS_ATTR_CNID);
> >  	if (!sbi->attr_tree)
> >  		pr_err("failed to load attributes file\n");
> > +	else
> > +		hfsplus_mark_inode_dirty(HFSPLUS_ATTR_TREE_I(sb),
> > +					 HFSPLUS_I_ATTR_DIRTY);
> 
> As far as I can see, HFSPLUS_ATTR_TREE_I(sb) and attr_file are the same
> entities. Am I right here? :) So, we can simply remove the first
Yes, they point to the same attr inode.
> hfsplus_mark_inode_dirty(). Does it make sense?
Hmm, yes.

Edward
BR

[PATCH v2] hfsplus: Remove the duplicate attr inode dirty marking action

Posted by Edward Adam Davis 2 months ago

Syzbot reported a null-ptr-deref in [1].
If the attributes file is not loaded during system mount, a trigger
occurs [1] when setxattr is executed in userspace.

Remove the first mark attr inode dirty operation.

[1]
KASAN: null-ptr-deref in range [0x0000000000000008-0x000000000000000f]
Call Trace:
 hfsplus_setxattr+0x124/0x340 fs/hfsplus/xattr.c:555
 hfsplus_trusted_setxattr+0x40/0x60 fs/hfsplus/xattr_trusted.c:30
 __vfs_setxattr+0x43c/0x480 fs/xattr.c:218
 __vfs_setxattr_noperm+0x12d/0x660 fs/xattr.c:252
 vfs_setxattr+0x163/0x360 fs/xattr.c:339
 do_setxattr fs/xattr.c:654 [inline]

Reported-by: syzbot+bc70a12e438dadba4fb4@syzkaller.appspotmail.com
Fixes: ee8422d00b7c ("hfsplus: fix potential Allocation File corruption after fsync")
Closes: https://syzkaller.appspot.com/bug?extid=bc70a12e438dadba4fb4
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
v1 -> v2: just remove first mark dirty

 fs/hfsplus/xattr.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
index 452a1f9becb2..21a1c196c71f 100644
--- a/fs/hfsplus/xattr.c
+++ b/fs/hfsplus/xattr.c
@@ -317,7 +317,6 @@ static int hfsplus_create_attributes_file(struct super_block *sb)
 		next_node++;
 	}
 
-	hfsplus_mark_inode_dirty(HFSPLUS_ATTR_TREE_I(sb), HFSPLUS_I_ATTR_DIRTY);
 	hfsplus_mark_inode_dirty(attr_file, HFSPLUS_I_ATTR_DIRTY);
 
 	sbi->attr_tree = hfs_btree_open(sb, HFSPLUS_ATTR_CNID);
-- 
2.43.0

Re: [PATCH v2] hfsplus: Remove the duplicate attr inode dirty marking action

Posted by Viacheslav Dubeyko 2 months ago

On Thu, 2026-04-16 at 11:37 +0800, Edward Adam Davis wrote:
> Syzbot reported a null-ptr-deref in [1].
> If the attributes file is not loaded during system mount, a trigger
> occurs [1] when setxattr is executed in userspace.
> 
> Remove the first mark attr inode dirty operation.
> 
> [1]
> KASAN: null-ptr-deref in range [0x0000000000000008-0x000000000000000f]
> Call Trace:
>  hfsplus_setxattr+0x124/0x340 fs/hfsplus/xattr.c:555
>  hfsplus_trusted_setxattr+0x40/0x60 fs/hfsplus/xattr_trusted.c:30
>  __vfs_setxattr+0x43c/0x480 fs/xattr.c:218
>  __vfs_setxattr_noperm+0x12d/0x660 fs/xattr.c:252
>  vfs_setxattr+0x163/0x360 fs/xattr.c:339
>  do_setxattr fs/xattr.c:654 [inline]
> 
> Reported-by: syzbot+bc70a12e438dadba4fb4@syzkaller.appspotmail.com
> Fixes: ee8422d00b7c ("hfsplus: fix potential Allocation File corruption after fsync")
> Closes: https://syzkaller.appspot.com/bug?extid=bc70a12e438dadba4fb4
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> ---
> v1 -> v2: just remove first mark dirty
> 
>  fs/hfsplus/xattr.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
> index 452a1f9becb2..21a1c196c71f 100644
> --- a/fs/hfsplus/xattr.c
> +++ b/fs/hfsplus/xattr.c
> @@ -317,7 +317,6 @@ static int hfsplus_create_attributes_file(struct super_block *sb)
>  		next_node++;
>  	}
>  
> -	hfsplus_mark_inode_dirty(HFSPLUS_ATTR_TREE_I(sb), HFSPLUS_I_ATTR_DIRTY);
>  	hfsplus_mark_inode_dirty(attr_file, HFSPLUS_I_ATTR_DIRTY);
>  
>  	sbi->attr_tree = hfs_btree_open(sb, HFSPLUS_ATTR_CNID);

Looks good.

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

Thanks,
Slava.