Re: [PATCH v3] hfsplus: fix null-ptr-deref in hfsplus_cat_write_inode

[Viacheslav Dubeyko]

On Wed, 2026-04-15 at 07:13 +0530, Deepanshu Kartikey wrote:
> When a corrupt HFS+ image is mounted where the hidden directory is
> absent, hfsplus_fill_super() handles -ENOENT from
> hfsplus_get_hidden_dir_entry() silently and continues with
> sbi->hidden_dir left as NULL.
> 
> hfsplus_link() and hfsplus_unlink() call
> hfsplus_cat_write_inode(sbi->hidden_dir) unconditionally without
> checking whether hidden_dir is NULL, triggering a general protection
> fault caught by KASAN as a null-ptr-deref at offset 0x28.
> 
> Other call sites in dir.c already guard against this by checking
> hidden_dir for NULL before use. Apply the same guard to the two
> unprotected call sites in hfsplus_link() and hfsplus_unlink().
> 
> Changes in v3:
>   - Correct fix location: guard sbi->hidden_dir before calling
>     hfsplus_cat_write_inode() in hfsplus_link() and hfsplus_unlink()
>     in dir.c, as suggested by Vyacheslav Dubeyko.
> 
> Changes in v2:
>   - Fixed commit message: hfsplus_delete_cat() is called from
>     multiple callers, not just hfsplus_unlink() as incorrectly
>     stated in v1.
> 
> Reported-by: syzbot+c0ba772a362e70937dfb@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=c0ba772a362e70937dfb
> Tested-by: syzbot+c0ba772a362e70937dfb@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Deepanshu Kartikey <kartikey406@xxxxxxxxx>
> ---
>  fs/hfsplus/dir.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
> index d559bf8625f8..a7feef53d8cb 100644
> --- a/fs/hfsplus/dir.c
> +++ b/fs/hfsplus/dir.c
> @@ -362,9 +362,11 @@ static int hfsplus_link(struct dentry *src_dentry, struct inode *dst_dir,
>  	if (res)
>  		goto out;
>  
> -	res = hfsplus_cat_write_inode(sbi->hidden_dir);
> -	if (res)
> -		goto out;
> +	if (sbi->hidden_dir) {
> +		res = hfsplus_cat_write_inode(sbi->hidden_dir);
> +		if (res)
> +			goto out;
> +	}
>  
>  	res = hfsplus_cat_write_inode(inode);
>  
> @@ -431,11 +433,10 @@ static int hfsplus_unlink(struct inode *dir, struct dentry *dentry)
>  out:
>  	if (!res) {
>  		res = hfsplus_cat_write_inode(dir);
> -		if (!res) {
> +		if (!res && sbi->hidden_dir)
>  			res = hfsplus_cat_write_inode(sbi->hidden_dir);
> -			if (!res)
> -				res = hfsplus_cat_write_inode(inode);
> -		}
> +		if (!res)
> +			res = hfsplus_cat_write_inode(inode);
>  	}
>  
>  	mutex_unlock(&sbi->vh_mutex);


As far as I can see, we have such logic in hfsplus_fill_super() [1]:

if (!sb_rdonly(sb)) {
<skipped>
    if (!sbi->hidden_dir) {
        <create hidden dir>
    }
<skipped>
}

So, I have two questions:
(1) If we mounted volume in READ-ONLY mode initially and we re-mount it into
READ-WRITE mode, then we skip hidden directory creation. Should we process this
in hfsplus_reconfigure()?
(2) Should we create hidden directory if corrupted volume hasn't hidden
directory?

If we don't create the hidden dir, then we could have troubles in multiple
places: hfsplus_rename_cat() [2,3], hfsplus_delete_cat() [4,5]. It looks like
that we need to create the hidden dir. Otherwise, we could trigger the kernel
crash in multiple places.

Thanks,
Slava.

[1] https://elixir.bootlin.com/linux/v7.0/source/fs/hfsplus/super.c#L589
[2] https://elixir.bootlin.com/linux/v7.0/source/fs/hfsplus/dir.c#L326
[3] https://elixir.bootlin.com/linux/v7.0/source/fs/hfsplus/dir.c#L397
[4] https://elixir.bootlin.com/linux/v7.0/source/fs/hfsplus/dir.c#L418
[5] https://elixir.bootlin.com/linux/v7.0/source/fs/hfsplus/inode.c#L257