Re: [PATCH v6] hfsplus: validate B-tree record offset table

[Viacheslav Dubeyko]

On Sat, 2026-08-08 at 22:50 +0800, Jiaming Zhang wrote:
> > 

<skipped>

> > I've realized that we have additional issue in HFS+ logic.
> > Potentially,
> > keylen and len could be not invalid but we could have negative
> > value
> > later in the logic:
> > 
> > done:
> >         fd->record = e;
> >         fd->keyoffset = off;
> >         fd->keylength = keylen;
> >         fd->entryoffset = off + keylen;
> >         fd->entrylength = len - keylen; <-- negative value here.
> > 
> > The negative value of fd->entrylength is checked in multiple
> > places.
> > However, there is extents tree logic that has no such check [1]:
> > 
> > static int __hfsplus_ext_write_extent(struct inode *inode,
> >                 struct hfs_find_data *fd)
> > {
> > <skipped>
> > 
> >         } else {
> >                 if (res)
> >                         return res;
> >                 hfs_bnode_write(fd->bnode, hip->cached_extents,
> >                                 fd->entryoffset, fd->entrylength);
> >                 hip->extent_state &= ~HFSPLUS_EXT_DIRTY;
> >         }
> > 
> > <skipped>
> > }
> > 
> > Could you please double check my conclusion? We can fix the issue
> > in
> > another patch. Could you please fix the issue?
> > 
> 
> I agree with your conclusion. A crafted image can keep both len and
> keylen valid but make fd->entrylength negative (i.e. keylen > len),
> which may lead to out-of-bound read and kernel memory leaking.
> 
> To fix this issue, we can check validity of fd->entrylength in
> __hfsplus_ext_write_extent() like the check in
> __hfsplus_ext_read_extent(). For example:
> 
> diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c
> index 813e68b8ecd6..eb7c11524d18 100644
> --- a/fs/hfsplus/extents.c
> +++ b/fs/hfsplus/extents.c
> @@ -110,6 +110,8 @@ static int __hfsplus_ext_write_extent(struct
> inode *inode,
>         } else {
>                 if (res)
>                         return res;
> +               if (fd->entrylength != sizeof(hfsplus_extent_rec))
> +                       return -EIO;
>                 hfs_bnode_write(fd->bnode, hip->cached_extents,
>                                 fd->entryoffset, fd->entrylength);
>                 hip->extent_state &= ~HFSPLUS_EXT_DIRTY;
> 
> Do you think this fix is acceptable? If so, I'm happy to send another
> patch.
> 

As far as I can see, the fix makes sense.

Thanks,
Slava.