Re: [PATCH v6] hfsplus: fix null-ptr-deref by creating hidden dir on remount rw
From: Viacheslav Dubeyko
Date: Tue Jul 14 2026 - 17:33:14 EST
On Tue, 2026-07-14 at 18:26 +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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=c0ba772a362e70937dfb
> Signed-off-by: Deepanshu Kartikey <kartikey406@xxxxxxxxx>
> ---
> 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 both fill_super and reconfigure.
> - 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 | 100 ++++++++++++++++++++++++++++---------------
> --
> 1 file changed, 62 insertions(+), 38 deletions(-)
>
> diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
> index 40a0feda716b..56138881c35f 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,18 @@ 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) && !sbi->hidden_dir)
> {
> + struct qstr str =
> QSTR_INIT(HFSP_HIDDENDIR_NAME,
> +
> sizeof(HFSP_HIDDENDIR_NAME) - 1);
> + hfsplus_prepare_volume_header_for_commit(vhd
> r);
> + hfsplus_sync_fs(sb, 1);
I think that true is better than 1 for hfsplus_sync_fs() call.
So, we didn't call hfsplus_prepare_volume_header_for_commit() and
hfsplus_sync_fs() in hfsplus_reconfigure() before. But, probably, it's
right thing to do. But currently, we call it only for the case of !sbi-
>hidden_dir. Should we do it for every RW mount?
Thanks,
Slava.
> + return hfsplus_create_hidden_dir(sb, &str);
> + }
> }
> return 0;
> }
> @@ -589,8 +647,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 +678,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 +688,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;