* [PATCH v4] hfsplus: fix null-ptr-deref by creating hidden dir on remount rw
@ 2026-04-30 2:03 Deepanshu Kartikey
2026-04-30 19:46 ` Viacheslav Dubeyko
0 siblings, 1 reply; 3+ messages in thread
From: Deepanshu Kartikey @ 2026-04-30 2:03 UTC (permalink / raw)
To: slava, glaubitz, frank.li
Cc: linux-fsdevel, linux-kernel, Deepanshu Kartikey,
syzbot+c0ba772a362e70937dfb
hfsplus_reconfigure() does not create the hidden directory when
remounting from read-only to read-write, leaving sbi->hidden_dir
as NULL. This causes a null-ptr-deref when any subsequent
link/unlink/rename operation dereferences it.
Extract hidden directory creation into a helper and call it from
hfsplus_reconfigure() when switching to read-write mode.
Reported-by: syzbot+c0ba772a362e70937dfb@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=c0ba772a362e70937dfb
Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
---
Changes in v4:
- Correct fix: extract hidden dir creation into helper and call
from hfsplus_reconfigure() on remount rw, as suggested by
Vyacheslav Dubeyko.
Changes in v3:
- Correct fix location: guard sbi->hidden_dir in hfsplus_link()
and hfsplus_unlink() in dir.c.
Changes in v2:
- Fixed commit message: hfsplus_delete_cat() has multiple callers,
not just hfsplus_unlink() as incorrectly stated in v1.
---
fs/hfsplus/super.c | 88 ++++++++++++++++++++++++++++------------------
1 file changed, 53 insertions(+), 35 deletions(-)
diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index 7229a8ae89f9..8b5c39ce4d48 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -372,15 +372,59 @@ static int hfsplus_statfs(struct dentry *dentry, struct kstatfs *buf)
return 0;
}
+static int hfsplus_create_hidden_dir(struct super_block *sb)
+{
+ struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
+ struct inode *root = d_inode(sb->s_root);
+ struct qstr str;
+ int err;
+
+ str.len = sizeof(HFSP_HIDDENDIR_NAME) - 1;
+ str.name = HFSP_HIDDENDIR_NAME;
+
+ mutex_lock(&sbi->vh_mutex);
+ sbi->hidden_dir = hfsplus_new_inode(sb, root, S_IFDIR);
+ if (!sbi->hidden_dir) {
+ mutex_unlock(&sbi->vh_mutex);
+ return -ENOMEM;
+ }
+
+ err = hfsplus_create_cat(sbi->hidden_dir->i_ino, root,
+ &str, sbi->hidden_dir);
+ if (err) {
+ mutex_unlock(&sbi->vh_mutex);
+ goto out_put_hidden_dir;
+ }
+
+ err = hfsplus_init_security(sbi->hidden_dir, root, &str);
+ if (err == -EOPNOTSUPP)
+ err = 0;
+ else if (err) {
+ hfsplus_delete_cat(sbi->hidden_dir->i_ino, root, &str);
+ mutex_unlock(&sbi->vh_mutex);
+ goto out_put_hidden_dir;
+ }
+
+ mutex_unlock(&sbi->vh_mutex);
+ hfsplus_mark_inode_dirty(sbi->hidden_dir,
+ HFSPLUS_I_CAT_DIRTY);
+ return 0;
+
+out_put_hidden_dir:
+ iput(sbi->hidden_dir);
+ sbi->hidden_dir = NULL;
+ return err;
+}
+
static int hfsplus_reconfigure(struct fs_context *fc)
{
struct super_block *sb = fc->root->d_sb;
+ struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
sync_filesystem(sb);
if ((bool)(fc->sb_flags & SB_RDONLY) == sb_rdonly(sb))
return 0;
if (!(fc->sb_flags & SB_RDONLY)) {
- struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
struct hfsplus_vh *vhdr = sbi->s_vhdr;
if (!(vhdr->attributes & cpu_to_be32(HFSPLUS_VOL_UNMNT))) {
@@ -401,6 +445,12 @@ static int hfsplus_reconfigure(struct fs_context *fc)
fc->sb_flags |= SB_RDONLY;
}
}
+ /*
+ * Create hidden dir if remounting read-write and it does
+ * not exist - required for link/unlink/rename operations.
+ */
+ if (!sb_rdonly(sb) && !sbi->hidden_dir)
+ return hfsplus_create_hidden_dir(sb);
return 0;
}
@@ -595,38 +645,9 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
hfsplus_sync_fs(sb, 1);
if (!sbi->hidden_dir) {
- mutex_lock(&sbi->vh_mutex);
- sbi->hidden_dir = hfsplus_new_inode(sb, root, S_IFDIR);
- if (!sbi->hidden_dir) {
- mutex_unlock(&sbi->vh_mutex);
- err = -ENOMEM;
+ err = hfsplus_create_hidden_dir(sb);
+ if (err)
goto out_put_root;
- }
- err = hfsplus_create_cat(sbi->hidden_dir->i_ino, root,
- &str, sbi->hidden_dir);
- if (err) {
- mutex_unlock(&sbi->vh_mutex);
- goto out_put_hidden_dir;
- }
-
- err = hfsplus_init_security(sbi->hidden_dir,
- root, &str);
- if (err == -EOPNOTSUPP)
- err = 0; /* Operation is not supported. */
- else if (err) {
- /*
- * Try to delete anyway without
- * error analysis.
- */
- hfsplus_delete_cat(sbi->hidden_dir->i_ino,
- root, &str);
- mutex_unlock(&sbi->vh_mutex);
- goto out_put_hidden_dir;
- }
-
- mutex_unlock(&sbi->vh_mutex);
- hfsplus_mark_inode_dirty(sbi->hidden_dir,
- HFSPLUS_I_CAT_DIRTY);
}
}
@@ -634,9 +655,6 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
sbi->nls = nls;
return 0;
-out_put_hidden_dir:
- cancel_delayed_work_sync(&sbi->sync_work);
- iput(sbi->hidden_dir);
out_put_root:
dput(sb->s_root);
sb->s_root = NULL;
--
2.43.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v4] hfsplus: fix null-ptr-deref by creating hidden dir on remount rw
2026-04-30 2:03 [PATCH v4] hfsplus: fix null-ptr-deref by creating hidden dir on remount rw Deepanshu Kartikey
@ 2026-04-30 19:46 ` Viacheslav Dubeyko
2026-05-02 14:49 ` Deepanshu Kartikey
0 siblings, 1 reply; 3+ messages in thread
From: Viacheslav Dubeyko @ 2026-04-30 19:46 UTC (permalink / raw)
To: Deepanshu Kartikey, slava, glaubitz, frank.li
Cc: linux-fsdevel, linux-kernel, syzbot+c0ba772a362e70937dfb
On Thu, 2026-04-30 at 07:33 +0530, Deepanshu Kartikey wrote:
> hfsplus_reconfigure() does not create the hidden directory when
> remounting from read-only to read-write, leaving sbi->hidden_dir
> as NULL. This causes a null-ptr-deref when any subsequent
> link/unlink/rename operation dereferences it.
>
> Extract hidden directory creation into a helper and call it from
> hfsplus_reconfigure() when switching to read-write mode.
>
> Reported-by: syzbot+c0ba772a362e70937dfb@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=c0ba772a362e70937dfb
> Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
> ---
> Changes in v4:
> - Correct fix: extract hidden dir creation into helper and call
> from hfsplus_reconfigure() on remount rw, as suggested by
> Vyacheslav Dubeyko.
>
> Changes in v3:
> - Correct fix location: guard sbi->hidden_dir in hfsplus_link()
> and hfsplus_unlink() in dir.c.
>
> Changes in v2:
> - Fixed commit message: hfsplus_delete_cat() has multiple callers,
> not just hfsplus_unlink() as incorrectly stated in v1.
> ---
> fs/hfsplus/super.c | 88 ++++++++++++++++++++++++++++------------------
> 1 file changed, 53 insertions(+), 35 deletions(-)
>
> diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
> index 7229a8ae89f9..8b5c39ce4d48 100644
> --- a/fs/hfsplus/super.c
> +++ b/fs/hfsplus/super.c
> @@ -372,15 +372,59 @@ static int hfsplus_statfs(struct dentry *dentry, struct kstatfs *buf)
> return 0;
> }
>
> +static int hfsplus_create_hidden_dir(struct super_block *sb)
> +{
> + struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
> + struct inode *root = d_inode(sb->s_root);
> + struct qstr str;
> + int err;
> +
> + str.len = sizeof(HFSP_HIDDENDIR_NAME) - 1;
> + str.name = HFSP_HIDDENDIR_NAME;
The hfsplus_fill_super() has this initialization. It looks like duplication.
Maybe, we need to add this into hfsplus_reconfigure() and get str as input
argument?
> +
> + mutex_lock(&sbi->vh_mutex);
> + sbi->hidden_dir = hfsplus_new_inode(sb, root, S_IFDIR);
> + if (!sbi->hidden_dir) {
> + mutex_unlock(&sbi->vh_mutex);
> + return -ENOMEM;
> + }
> +
> + err = hfsplus_create_cat(sbi->hidden_dir->i_ino, root,
> + &str, sbi->hidden_dir);
> + if (err) {
> + mutex_unlock(&sbi->vh_mutex);
> + goto out_put_hidden_dir;
> + }
> +
> + err = hfsplus_init_security(sbi->hidden_dir, root, &str);
> + if (err == -EOPNOTSUPP)
> + err = 0;
> + else if (err) {
> + hfsplus_delete_cat(sbi->hidden_dir->i_ino, root, &str);
> + mutex_unlock(&sbi->vh_mutex);
> + goto out_put_hidden_dir;
> + }
> +
> + mutex_unlock(&sbi->vh_mutex);
> + hfsplus_mark_inode_dirty(sbi->hidden_dir,
> + HFSPLUS_I_CAT_DIRTY);
> + return 0;
> +
> +out_put_hidden_dir:
> + iput(sbi->hidden_dir);
> + sbi->hidden_dir = NULL;
> + return err;
> +}
> +
> static int hfsplus_reconfigure(struct fs_context *fc)
> {
> struct super_block *sb = fc->root->d_sb;
> + struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
>
> sync_filesystem(sb);
> if ((bool)(fc->sb_flags & SB_RDONLY) == sb_rdonly(sb))
> return 0;
> if (!(fc->sb_flags & SB_RDONLY)) {
> - struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
> struct hfsplus_vh *vhdr = sbi->s_vhdr;
>
> if (!(vhdr->attributes & cpu_to_be32(HFSPLUS_VOL_UNMNT))) {
> @@ -401,6 +445,12 @@ static int hfsplus_reconfigure(struct fs_context *fc)
> fc->sb_flags |= SB_RDONLY;
> }
> }
> + /*
> + * Create hidden dir if remounting read-write and it does
> + * not exist - required for link/unlink/rename operations.
> + */
> + if (!sb_rdonly(sb) && !sbi->hidden_dir)
> + return hfsplus_create_hidden_dir(sb);
It looks like hfsplus_reconfigure() is always false on a successful remount. The
sb->s_flags is updated by the VFS after reconfigure returns 0, not inside it.
The correct guard is !(fc->sb_flags & SB_RDONLY), which reflects the negotiated
new state.
> return 0;
> }
>
> @@ -595,38 +645,9 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
> hfsplus_sync_fs(sb, 1);
>
> if (!sbi->hidden_dir) {
> - mutex_lock(&sbi->vh_mutex);
> - sbi->hidden_dir = hfsplus_new_inode(sb, root, S_IFDIR);
> - if (!sbi->hidden_dir) {
> - mutex_unlock(&sbi->vh_mutex);
> - err = -ENOMEM;
> + err = hfsplus_create_hidden_dir(sb);
> + if (err)
> goto out_put_root;
> - }
> - err = hfsplus_create_cat(sbi->hidden_dir->i_ino, root,
> - &str, sbi->hidden_dir);
> - if (err) {
> - mutex_unlock(&sbi->vh_mutex);
> - goto out_put_hidden_dir;
> - }
> -
> - err = hfsplus_init_security(sbi->hidden_dir,
> - root, &str);
> - if (err == -EOPNOTSUPP)
> - err = 0; /* Operation is not supported. */
> - else if (err) {
> - /*
> - * Try to delete anyway without
> - * error analysis.
> - */
What was wrong with the comment? Why have we lost it?
> - hfsplus_delete_cat(sbi->hidden_dir->i_ino,
> - root, &str);
> - mutex_unlock(&sbi->vh_mutex);
> - goto out_put_hidden_dir;
> - }
> -
> - mutex_unlock(&sbi->vh_mutex);
> - hfsplus_mark_inode_dirty(sbi->hidden_dir,
> - HFSPLUS_I_CAT_DIRTY);
Are you sure that you work with the latest sources? Because, current state of
this code is:
hfsplus_mark_inode_dirty(HFSPLUS_CAT_TREE_I(sb), HFSPLUS_I_CAT_DIRTY);
hfsplus_mark_inode_dirty(sbi->hidden_dir, HFSPLUS_I_CAT_DIRTY);
> }
> }
>
> @@ -634,9 +655,6 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
> sbi->nls = nls;
> return 0;
>
> -out_put_hidden_dir:
> - cancel_delayed_work_sync(&sbi->sync_work);
Why cancel_delayed_work_sync() has been removed? The hfsplus_new_inode() calls
hfsplus_mark_mdb_dirty() which queues sync_work. So, nothing cancels the queued
work in the case of any issue.
Thanks,
Slava.
> - iput(sbi->hidden_dir);
> out_put_root:
> dput(sb->s_root);
> sb->s_root = NULL;
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v4] hfsplus: fix null-ptr-deref by creating hidden dir on remount rw
2026-04-30 19:46 ` Viacheslav Dubeyko
@ 2026-05-02 14:49 ` Deepanshu Kartikey
0 siblings, 0 replies; 3+ messages in thread
From: Deepanshu Kartikey @ 2026-05-02 14:49 UTC (permalink / raw)
To: Viacheslav Dubeyko
Cc: slava, glaubitz, frank.li, linux-fsdevel, linux-kernel,
syzbot+c0ba772a362e70937dfb
On Fri, May 1, 2026 at 1:16 AM Viacheslav Dubeyko <vdubeyko@redhat.com> wrote:
>
> On Thu, 2026-04-30 at 07:33 +0530, Deepanshu Kartikey wrote:
> > hfsplus_reconfigure() does not create the hidden directory when
> > remounting from read-only to read-write, leaving sbi->hidden_dir
> > as NULL. This causes a null-ptr-deref when any subsequent
> > link/unlink/rename operation dereferences it.
> >
> > Extract hidden directory creation into a helper and call it from
> > hfsplus_reconfigure() when switching to read-write mode.
> >
> > Reported-by: syzbot+c0ba772a362e70937dfb@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=c0ba772a362e70937dfb
> > Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
> > ---
> > Changes in v4:
> > - Correct fix: extract hidden dir creation into helper and call
> > from hfsplus_reconfigure() on remount rw, as suggested by
> > Vyacheslav Dubeyko.
> >
> > Changes in v3:
> > - Correct fix location: guard sbi->hidden_dir in hfsplus_link()
> > and hfsplus_unlink() in dir.c.
> >
> > Changes in v2:
> > - Fixed commit message: hfsplus_delete_cat() has multiple callers,
> > not just hfsplus_unlink() as incorrectly stated in v1.
> > ---
> > fs/hfsplus/super.c | 88 ++++++++++++++++++++++++++++------------------
> > 1 file changed, 53 insertions(+), 35 deletions(-)
> >
> > diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
> > index 7229a8ae89f9..8b5c39ce4d48 100644
> > --- a/fs/hfsplus/super.c
> > +++ b/fs/hfsplus/super.c
> > @@ -372,15 +372,59 @@ static int hfsplus_statfs(struct dentry *dentry, struct kstatfs *buf)
> > return 0;
> > }
> >
> > +static int hfsplus_create_hidden_dir(struct super_block *sb)
> > +{
> > + struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
> > + struct inode *root = d_inode(sb->s_root);
> > + struct qstr str;
> > + int err;
> > +
> > + str.len = sizeof(HFSP_HIDDENDIR_NAME) - 1;
> > + str.name = HFSP_HIDDENDIR_NAME;
>
> The hfsplus_fill_super() has this initialization. It looks like duplication.
> Maybe, we need to add this into hfsplus_reconfigure() and get str as input
> argument?
>
> > +
> > + mutex_lock(&sbi->vh_mutex);
> > + sbi->hidden_dir = hfsplus_new_inode(sb, root, S_IFDIR);
> > + if (!sbi->hidden_dir) {
> > + mutex_unlock(&sbi->vh_mutex);
> > + return -ENOMEM;
> > + }
> > +
> > + err = hfsplus_create_cat(sbi->hidden_dir->i_ino, root,
> > + &str, sbi->hidden_dir);
> > + if (err) {
> > + mutex_unlock(&sbi->vh_mutex);
> > + goto out_put_hidden_dir;
> > + }
> > +
> > + err = hfsplus_init_security(sbi->hidden_dir, root, &str);
> > + if (err == -EOPNOTSUPP)
> > + err = 0;
> > + else if (err) {
> > + hfsplus_delete_cat(sbi->hidden_dir->i_ino, root, &str);
> > + mutex_unlock(&sbi->vh_mutex);
> > + goto out_put_hidden_dir;
> > + }
> > +
> > + mutex_unlock(&sbi->vh_mutex);
> > + hfsplus_mark_inode_dirty(sbi->hidden_dir,
> > + HFSPLUS_I_CAT_DIRTY);
> > + return 0;
> > +
> > +out_put_hidden_dir:
> > + iput(sbi->hidden_dir);
> > + sbi->hidden_dir = NULL;
> > + return err;
> > +}
> > +
> > static int hfsplus_reconfigure(struct fs_context *fc)
> > {
> > struct super_block *sb = fc->root->d_sb;
> > + struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
> >
> > sync_filesystem(sb);
> > if ((bool)(fc->sb_flags & SB_RDONLY) == sb_rdonly(sb))
> > return 0;
> > if (!(fc->sb_flags & SB_RDONLY)) {
> > - struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
> > struct hfsplus_vh *vhdr = sbi->s_vhdr;
> >
> > if (!(vhdr->attributes & cpu_to_be32(HFSPLUS_VOL_UNMNT))) {
> > @@ -401,6 +445,12 @@ static int hfsplus_reconfigure(struct fs_context *fc)
> > fc->sb_flags |= SB_RDONLY;
> > }
> > }
> > + /*
> > + * Create hidden dir if remounting read-write and it does
> > + * not exist - required for link/unlink/rename operations.
> > + */
> > + if (!sb_rdonly(sb) && !sbi->hidden_dir)
> > + return hfsplus_create_hidden_dir(sb);
>
> It looks like hfsplus_reconfigure() is always false on a successful remount. The
> sb->s_flags is updated by the VFS after reconfigure returns 0, not inside it.
> The correct guard is !(fc->sb_flags & SB_RDONLY), which reflects the negotiated
> new state.
>
> > return 0;
> > }
> >
> > @@ -595,38 +645,9 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
> > hfsplus_sync_fs(sb, 1);
> >
> > if (!sbi->hidden_dir) {
> > - mutex_lock(&sbi->vh_mutex);
> > - sbi->hidden_dir = hfsplus_new_inode(sb, root, S_IFDIR);
> > - if (!sbi->hidden_dir) {
> > - mutex_unlock(&sbi->vh_mutex);
> > - err = -ENOMEM;
> > + err = hfsplus_create_hidden_dir(sb);
> > + if (err)
> > goto out_put_root;
> > - }
> > - err = hfsplus_create_cat(sbi->hidden_dir->i_ino, root,
> > - &str, sbi->hidden_dir);
> > - if (err) {
> > - mutex_unlock(&sbi->vh_mutex);
> > - goto out_put_hidden_dir;
> > - }
> > -
> > - err = hfsplus_init_security(sbi->hidden_dir,
> > - root, &str);
> > - if (err == -EOPNOTSUPP)
> > - err = 0; /* Operation is not supported. */
> > - else if (err) {
> > - /*
> > - * Try to delete anyway without
> > - * error analysis.
> > - */
>
> What was wrong with the comment? Why have we lost it?
>
> > - hfsplus_delete_cat(sbi->hidden_dir->i_ino,
> > - root, &str);
> > - mutex_unlock(&sbi->vh_mutex);
> > - goto out_put_hidden_dir;
> > - }
> > -
> > - mutex_unlock(&sbi->vh_mutex);
> > - hfsplus_mark_inode_dirty(sbi->hidden_dir,
> > - HFSPLUS_I_CAT_DIRTY);
>
> Are you sure that you work with the latest sources? Because, current state of
> this code is:
>
> hfsplus_mark_inode_dirty(HFSPLUS_CAT_TREE_I(sb), HFSPLUS_I_CAT_DIRTY);
> hfsplus_mark_inode_dirty(sbi->hidden_dir, HFSPLUS_I_CAT_DIRTY);
>
> > }
> > }
> >
> > @@ -634,9 +655,6 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
> > sbi->nls = nls;
> > return 0;
> >
> > -out_put_hidden_dir:
> > - cancel_delayed_work_sync(&sbi->sync_work);
>
> Why cancel_delayed_work_sync() has been removed? The hfsplus_new_inode() calls
> hfsplus_mark_mdb_dirty() which queues sync_work. So, nothing cancels the queued
> work in the case of any issue.
>
> Thanks,
> Slava.
>
> > - iput(sbi->hidden_dir);
> > out_put_root:
> > dput(sb->s_root);
> > sb->s_root = NULL;
>
I will send the patch v5 shortly
Thanks
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-05-02 14:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-30 2:03 [PATCH v4] hfsplus: fix null-ptr-deref by creating hidden dir on remount rw Deepanshu Kartikey
2026-04-30 19:46 ` Viacheslav Dubeyko
2026-05-02 14:49 ` Deepanshu Kartikey
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox