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

[Viacheslav Dubeyko]

On Sun, 2026-05-03 at 10:14 +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 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 | 97 +++++++++++++++++++++++++++++-----------------
>  1 file changed, 61 insertions(+), 36 deletions(-)
> 
> diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
> index 40a0feda716b..f37d5abc84cf 100644
> --- a/fs/hfsplus/super.c
> +++ b/fs/hfsplus/super.c
> @@ -375,6 +375,54 @@ 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) {
> +		mutex_unlock(&sbi->vh_mutex);

Frankly speaking, we have too many places of calling mutex_unlock() in the case
of error. It will be much better to call mutex_unlock() in one place for the
case of error. Could we rework this code pattern?

> +		return -ENOMEM;
> +	}
> +
> +	err = hfsplus_create_cat(sbi->hidden_dir->i_ino, root,
> +				 str, sbi->hidden_dir);
> +	if (err) {
> +		mutex_unlock(&sbi->vh_mutex);

Ditto.

> +		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);

Ditto.

> +		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);
> +	return 0;
> +
> +out_put_hidden_dir:

Here it will be enough to have the out label name. Because, we are inside of
hfsplus_create_hidden_dir() method.

> +	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 +451,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.
> +		 */

I started to think that, probably, we need this logic here:

  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(vhdr);                          
      hfsplus_sync_fs(sb, 1);                                                  
      return hfsplus_create_hidden_dir(sb, &str);                              
  }

In hfsplus_fill_super() we have:

  hfsplus_prepare_volume_header_for_commit(vhdr);
  hfsplus_sync_fs(sb, 1);

If the machine crashes mid-creation, the next mount sees HFSPLUS_VOL_UNMNT clear
and runs fsck. However, we didn't anything like this for the case of
hfsplus_reconfigure(). If the system crashes after the B-tree entry for the
hidden directory is written but before the next asynchronous hfsplus_sync_fs()
runs the volume header still has HFSPLUS_VOL_UNMNT set while the B-tree contains
the new entry. The next mount sees a "cleanly unmounted" header, accepts it, and
skips fsck — leaving the filesystem in a subtly inconsistent state. What do you
think? Am I right here?

> +		if (!(fc->sb_flags & SB_RDONLY) && !sbi->hidden_dir) {
> +			struct qstr str;
> +
> +			str.len = sizeof(HFSP_HIDDENDIR_NAME) - 1;
> +			str.name = HFSP_HIDDENDIR_NAME;

The str.hash (the other field of the union in struct qstr) is left
uninitialised. I think we need to do it in different way here and in
hfsplus_fill_super():

  struct qstr str = QSTR_INIT(HFSP_HIDDENDIR_NAME,
                              sizeof(HFSP_HIDDENDIR_NAME) - 1);

Thanks,
Slava.

> +			return hfsplus_create_hidden_dir(sb, &str);
> +		}
>  	}
>  	return 0;
>  }
> @@ -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;