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

[PATCH v7] 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 logic from hfsplus_fill_super()
into a new helper hfsplus_create_hidden_dir() and call it from
hfsplus_reconfigure() when switching to read-write mode and
hidden_dir is NULL, ensuring hidden_dir is always valid on any
read-write mount.

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 v7:
  - Call hfsplus_prepare_volume_header_for_commit() and
    hfsplus_sync_fs() for all rw remounts in reconfigure
    not just when hidden_dir is NULL, as suggested by
    Vyacheslav Dubeyko.
  - Remove redundant inner hidden_dir NULL check.

Changes in v6:
  - Use single mutex_unlock() in error path of helper.
  - Rename label out_put_hidden_dir to out inside helper.
  - Use QSTR_INIT() in reconfigure and compound literal
    cast in fill_super.
  - Add hfsplus_prepare_volume_header_for_commit() and
    hfsplus_sync_fs() before creating hidden dir in reconfigure
    to avoid inconsistent state on crash, as suggested by
    Vyacheslav Dubeyko.

Changes in v5:
  - Pass str as input argument to hfsplus_create_hidden_dir()
    to avoid duplication, as suggested by Vyacheslav Dubeyko.
  - Use !(fc->sb_flags & SB_RDONLY) as guard in reconfigure
    instead of !sb_rdonly(sb).
  - Restore cancel_delayed_work_sync() in cleanup path.
  - Restore HFSPLUS_CAT_TREE_I dirty mark.

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 | 102 ++++++++++++++++++++++++++++-----------------
 1 file changed, 64 insertions(+), 38 deletions(-)

diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index 40a0feda716b..3983e8160302 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -375,6 +375,52 @@ static int hfsplus_statfs(struct dentry *dentry, struct kstatfs *buf)
 	return 0;
 }
 
+static int hfsplus_create_hidden_dir(struct super_block *sb,
+				     const struct qstr *str)
+{
+	struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
+	struct inode *root = d_inode(sb->s_root);
+	int err;
+
+	mutex_lock(&sbi->vh_mutex);
+	sbi->hidden_dir = hfsplus_new_inode(sb, root, S_IFDIR);
+	if (!sbi->hidden_dir) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	err = hfsplus_create_cat(sbi->hidden_dir->i_ino, root,
+				 str, sbi->hidden_dir);
+	if (err)
+		goto out;
+
+	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);
+		goto out;
+	}
+
+	mutex_unlock(&sbi->vh_mutex);
+	hfsplus_mark_inode_dirty(HFSPLUS_CAT_TREE_I(sb),
+				 HFSPLUS_I_CAT_DIRTY);
+	hfsplus_mark_inode_dirty(sbi->hidden_dir,
+				 HFSPLUS_I_CAT_DIRTY);
+	return 0;
+
+out:
+	mutex_unlock(&sbi->vh_mutex);
+	cancel_delayed_work_sync(&sbi->sync_work);
+	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;
@@ -403,6 +449,20 @@ static int hfsplus_reconfigure(struct fs_context *fc)
 			sb->s_flags |= SB_RDONLY;
 			fc->sb_flags |= SB_RDONLY;
 		}
+
+		/*
+		 * Create hidden dir if remounting read-write and it
+		 * does not exist - required for link/unlink/rename.
+		 */
+		if (!(fc->sb_flags & SB_RDONLY)) {
+			hfsplus_prepare_volume_header_for_commit(vhdr);
+			hfsplus_sync_fs(sb, 1);
+			if (!sbi->hidden_dir) {
+				struct qstr str = QSTR_INIT(HFSP_HIDDENDIR_NAME,
+						    sizeof(HFSP_HIDDENDIR_NAME) - 1);
+				return hfsplus_create_hidden_dir(sb, &str);
+			}
+		}
 	}
 	return 0;
 }
@@ -589,8 +649,8 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
 		goto out_put_alloc_file;
 	}
 
-	str.len = sizeof(HFSP_HIDDENDIR_NAME) - 1;
-	str.name = HFSP_HIDDENDIR_NAME;
+	str = (struct qstr)QSTR_INIT(HFSP_HIDDENDIR_NAME,
+				     sizeof(HFSP_HIDDENDIR_NAME) - 1);
 	err = hfsplus_get_hidden_dir_entry(sb, &str, &entry);
 	if (err == -ENOENT) {
 		/*
@@ -620,40 +680,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, &str);
+			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(HFSPLUS_CAT_TREE_I(sb),
-						 HFSPLUS_I_CAT_DIRTY);
-			hfsplus_mark_inode_dirty(sbi->hidden_dir,
-						 HFSPLUS_I_CAT_DIRTY);
 		}
 	}
 
@@ -661,9 +690,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 v7] hfsplus: fix null-ptr-deref by creating hidden dir on remount rw

[Viacheslav Dubeyko]

On Fri, 2026-07-17 at 19:27 +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 logic from hfsplus_fill_super()
> into a new helper hfsplus_create_hidden_dir() and call it from
> hfsplus_reconfigure() when switching to read-write mode and
> hidden_dir is NULL, ensuring hidden_dir is always valid on any
> read-write mount.
> 
> 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 v7:
>   - Call hfsplus_prepare_volume_header_for_commit() and
>     hfsplus_sync_fs() for all rw remounts in reconfigure
>     not just when hidden_dir is NULL, as suggested by
>     Vyacheslav Dubeyko.
>   - Remove redundant inner hidden_dir NULL check.
> 
> Changes in v6:
>   - Use single mutex_unlock() in error path of helper.
>   - Rename label out_put_hidden_dir to out inside helper.
>   - Use QSTR_INIT() in reconfigure and compound literal
>     cast in fill_super.
>   - Add hfsplus_prepare_volume_header_for_commit() and
>     hfsplus_sync_fs() before creating hidden dir in reconfigure
>     to avoid inconsistent state on crash, as suggested by
>     Vyacheslav Dubeyko.
> 
> Changes in v5:
>   - Pass str as input argument to hfsplus_create_hidden_dir()
>     to avoid duplication, as suggested by Vyacheslav Dubeyko.
>   - Use !(fc->sb_flags & SB_RDONLY) as guard in reconfigure
>     instead of !sb_rdonly(sb).
>   - Restore cancel_delayed_work_sync() in cleanup path.
>   - Restore HFSPLUS_CAT_TREE_I dirty mark.
> 
> 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 | 102 ++++++++++++++++++++++++++++---------------
> --
>  1 file changed, 64 insertions(+), 38 deletions(-)
> 
> diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
> index 40a0feda716b..3983e8160302 100644
> --- a/fs/hfsplus/super.c
> +++ b/fs/hfsplus/super.c
> @@ -375,6 +375,52 @@ static int hfsplus_statfs(struct dentry *dentry,
> struct kstatfs *buf)
>  	return 0;
>  }
>  
> +static int hfsplus_create_hidden_dir(struct super_block *sb,
> +				     const struct qstr *str)
> +{
> +	struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
> +	struct inode *root = d_inode(sb->s_root);
> +	int err;
> +
> +	mutex_lock(&sbi->vh_mutex);
> +	sbi->hidden_dir = hfsplus_new_inode(sb, root, S_IFDIR);
> +	if (!sbi->hidden_dir) {
> +		err = -ENOMEM;
> +		goto out;
> +	}
> +
> +	err = hfsplus_create_cat(sbi->hidden_dir->i_ino, root,
> +				 str, sbi->hidden_dir);
> +	if (err)
> +		goto out;
> +
> +	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);
> +		goto out;
> +	}
> +
> +	mutex_unlock(&sbi->vh_mutex);
> +	hfsplus_mark_inode_dirty(HFSPLUS_CAT_TREE_I(sb),
> +				 HFSPLUS_I_CAT_DIRTY);
> +	hfsplus_mark_inode_dirty(sbi->hidden_dir,
> +				 HFSPLUS_I_CAT_DIRTY);
> +	return 0;
> +
> +out:
> +	mutex_unlock(&sbi->vh_mutex);
> +	cancel_delayed_work_sync(&sbi->sync_work);

Another issue we still have here. The hfsplus_create_hidden_dir()'s
failure path calls cancel_delayed_work_sync(&sbi->sync_work)
unconditionally. That's correct when called from hfsplus_fill_super()
(mount failing → whole sbi is being torn down anyway), but wrong when
called from hfsplus_reconfigure(). The sbi->work_queued is only ever
reset to 0 inside delayed_sync_fs() itself when it actually runs. If we
cancel a pending instance before it fires, work_queued stays stuck at 1
forever. I assume that the helper needs to not call
cancel_delayed_work_sync(). Does it make sense?

> +	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;
> @@ -403,6 +449,20 @@ static int hfsplus_reconfigure(struct fs_context
> *fc)
>  			sb->s_flags |= SB_RDONLY;
>  			fc->sb_flags |= SB_RDONLY;
>  		}
> +
> +		/*
> +		 * Create hidden dir if remounting read-write and it
> +		 * does not exist - required for link/unlink/rename.
> +		 */
> +		if (!(fc->sb_flags & SB_RDONLY)) {

I've spent more time on thinking about RW->RO remounting. And I think
we still have a serious issue here.

The "mark this volume cleanly unmounted" step (vhdr->attributes |=
HFSPLUS_VOL_UNMNT; ... &= ~HFSPLUS_VOL_INCNSTNT;) only happens in two
places: hfsplus_prepare_volume_header_for_commit() (called when
mounting or remounting rw) and directly inside hfsplus_put_super().

So take the sequence mount rw → remount ro → unmount: the volume was
marked INCNSTNT at mount time, never gets cleared by the remount-to-ro
(no code path does it), and then put_super() sees sb_rdonly(sb) == true
and skips clearing it too. The volume permanently retains
HFSPLUS_VOL_INCNSTNT / missing HFSPLUS_VOL_UNMNT after any such
session, so the next mount anywhere (Linux or macOS) will warn "not
cleanly unmounted, recommend fsck" and potentially force read-only - a
false positive, since nothing was actually left inconsistent.

So, sorry, but we need to rework the hfsplus_reconfigure() more
carefully. This patch revealed the pre-existing issue.

Thanks,
Slava.

> +			hfsplus_prepare_volume_header_for_commit(vhd
> r);
> +			hfsplus_sync_fs(sb, 1);
> +			if (!sbi->hidden_dir) {
> +				struct qstr str =
> QSTR_INIT(HFSP_HIDDENDIR_NAME,
> +						   
> sizeof(HFSP_HIDDENDIR_NAME) - 1);
> +				return hfsplus_create_hidden_dir(sb,
> &str);
> +			}
> +		}
>  	}
>  	return 0;
>  }
> @@ -589,8 +649,8 @@ static int hfsplus_fill_super(struct super_block
> *sb, struct fs_context *fc)
>  		goto out_put_alloc_file;
>  	}
>  
> -	str.len = sizeof(HFSP_HIDDENDIR_NAME) - 1;
> -	str.name = HFSP_HIDDENDIR_NAME;
> +	str = (struct qstr)QSTR_INIT(HFSP_HIDDENDIR_NAME,
> +				     sizeof(HFSP_HIDDENDIR_NAME) -
> 1);
>  	err = hfsplus_get_hidden_dir_entry(sb, &str, &entry);
>  	if (err == -ENOENT) {
>  		/*
> @@ -620,40 +680,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, &str);
> +			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(HFSPLUS_CAT_TREE_I(sb),
> -						
> HFSPLUS_I_CAT_DIRTY);
> -			hfsplus_mark_inode_dirty(sbi->hidden_dir,
> -						
> HFSPLUS_I_CAT_DIRTY);
>  		}
>  	}
>  
> @@ -661,9 +690,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;