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

[Viacheslav Dubeyko <vdubeyko@redhat.com>]

On Tue, 2026-04-21 at 07:14 +0530, Deepanshu Kartikey wrote:
> On Thu, Apr 16, 2026 at 3:18 AM Viacheslav Dubeyko <vdubeyko@redhat.com> wrote:
> > 
> > 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
> > 
> 
> Hi Vyacheslav,
> 
> Thank you for the detailed feedback and for pointing out the deeper
> design issue.
> 
> You are absolutely right on both counts.
> 
> (1) hfsplus_reconfigure() does not create the hidden directory when
> remounting from read-only to read-write. This means sbi->hidden_dir
> can be NULL on a writable mount even if the volume is otherwise
> healthy. This needs to be fixed in hfsplus_reconfigure().
> 
> (2) The correct approach is not to add NULL checks at individual
> call sites — that is a band-aid. The real fix is to ensure
> sbi->hidden_dir is always valid on any read-write mount, enforcing
> this invariant at mount and remount time.
> 
> My plan for v4:
> - Extract the hidden directory creation logic from hfsplus_fill_super()
>   into a helper function hfsplus_create_hidden_dir().
> - Call it from hfsplus_fill_super() as before.
> - Also call it from hfsplus_reconfigure() when switching from
>   read-only to read-write and hidden_dir is NULL.
> 
> This way hidden_dir is guaranteed valid for all write operations
> and the individual call sites in dir.c and inode.c are safe
> without needing NULL guards.
> 

Makes sense.

> I will send v4 shortly.
> 
> 

Sounds good.

Thanks,
Slava.