[PATCH v4] hfsplus: fix null-ptr-deref by creating hidden dir on remount rw

[PATCH v4] hfsplus: fix null-ptr-deref by creating hidden dir on remount rw

[Deepanshu Kartikey]

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

Re: [PATCH v4] hfsplus: fix null-ptr-deref by creating hidden dir on remount rw

[Viacheslav Dubeyko]

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;

Re: [PATCH v4] hfsplus: fix null-ptr-deref by creating hidden dir on remount rw

[Deepanshu Kartikey]

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