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

Viacheslav Dubeyko <[email protected]> 于2026年8月8日周六 08:49写道：
>
> On Thu, 2026-08-06 at 15:33 +0800, Jiaming Zhang wrote:
> > A crafted HFS+ image can contain a corrupted B-tree node. The node
> > descriptor may contain a record count that does not fit in the node,
> > and
> > record offsets may be unordered, unaligned, outside the node, or
> > point into
> > the offset table itself.
> >
> > Several B-tree helpers consume these on-disk fields before validating
> > them:
> > hfs_bnode_dump() can walk past the offset table when num_recs is
> > corrupted,
> > hfs_brec_lenoff() can produce an underflowed length or a record range
> > that
> > overlaps the offset table. This can make the unlink/writeback path
> > repeatedly call hfs_bnode_read_u16() with invalid offsets while
> > holding the
> > HFS+ B-tree lock, producing a flood of "requested invalid offset"
> > messages.
> > Other writeback workers then block on tree->tree_lock and the system
> > reports tasks hung in hfsplus_write_inode().
> >
> > Validate num_recs against the node size before walking the record
> > offset
> > table. Reject record ranges that are unordered, unaligned, outside
> > the
> > node, or overlapping the offset table. Reject invalid record indexes
> > before
> > reading their offset entries, and avoid decrementing an already-zero
> > leaf_count.
> >
> > Closes:
> > https://lore.kernel.org/lkml/CANypQFb_2TqKGrztAXj5m0_v+QChxXDnQVeifzV8J25Vuju10Q@mail.gmail.com/
> > Assisted-by: Codex:gpt-5.5-xhigh
> > Signed-off-by: Jiaming Zhang <[email protected]>
> > ---
> > Changes in v6:
> > - hfs_bmap_free(): Drop offset checks and print error message when B-
> > tree
> >   map record length is invalid.
> >
> > Changes in v5:
> > - Switch helpers to invalid checker and invert callers.
> > - hfs_brec_offsets_invalid(): Drop the redundant offset-table
> > argument
> >   since it is already covered by the offset-table overlap check and
> > take
> >   just the two neighboring record offsets (off and next_off).
> > - hfs_brec_len_invalid(): reject a length against node_size.
> > - Use hfs_brec_len_invalid() for both length checks in
> > hfs_bmap_get_map_page().
> > - Validate the record offset in hfs_bmap_free().
> > - Check the __hfs_brec_find() return code in [2] and [3]. [1] and [4]
> > call
> >   it to find the insertion slot for a new index key after a split, -
> > ENOENT is
> >   one of expected, even -EINVAL the following hfs_brec_insert() just
> > inserts
> >   at slot 0 of a node already validated by hfs_bnode_find(), so it
> > stays
> >   in-bounds and cannot trigger the invalid-offset flood. Hence no
> > check was
> >   added.
> >
> > [1]
> > https://elixir.bootlin.com/linux/v7.2-rc3/source/fs/hfsplus/brec.c#L160
> > [2]
> > https://elixir.bootlin.com/linux/v7.2-rc3/source/fs/hfsplus/brec.c#L208
> > [3]
> > https://elixir.bootlin.com/linux/v7.2-rc3/source/fs/hfsplus/brec.c#L382
> > [4]
> > https://elixir.bootlin.com/linux/v7.2-rc3/source/fs/hfsplus/brec.c#L449
> >
> > Changes in v4:
> > - Rename hfs_find_reset() to hfs_find_result_init().
> > - Reset find result fields in __hfs_brec_find().
> > - Move num_recs validation next to descriptor field initialization.
> > - Rename hfs_brec_range_valid() to hfs_brec_offpair_valid().
> > - Use U16_MAX for invalid offset/len/keylen sentinels and update
> > callers.
> > - Add hfs_brec_len_valid() to check validity of len/keylen.
> > - Handle invalid B-tree map record lengths in hfs_bmap_get_map_page()
> >   and hfs_bmap_free().
> > - Return -EINVAL instead of -EIO for invalid remove cursor/leaf_count
> > state.
> >
> > Changes in v3:
> > - Drop the keylen == len check.
> > - Drop the explicit zero-record check in __hfs_brec_find().
> > - Move find cursor reset into hfs_find_reset() and call it from
> > hfs_find_init() and hfs_brec_find().
> > - Rename helper-local variables as suggested.
> >
> >  fs/hfsplus/bfind.c      | 23 ++++++------
> >  fs/hfsplus/bnode.c      | 16 ++++++---
> >  fs/hfsplus/brec.c       | 53 ++++++++++++++++++---------
> >  fs/hfsplus/btree.c      | 16 +++++++--
> >  fs/hfsplus/hfsplus_fs.h | 79
> > +++++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 155 insertions(+), 32 deletions(-)
> >
> > diff --git a/fs/hfsplus/bfind.c b/fs/hfsplus/bfind.c
> > index 9a55fa6d5294..ca9813f58a6d 100644
> > --- a/fs/hfsplus/bfind.c
> > +++ b/fs/hfsplus/bfind.c
> > @@ -18,6 +18,7 @@ int hfs_find_init(struct hfs_btree *tree, struct
> > hfs_find_data *fd)
> >
> >       fd->tree = tree;
> >       fd->bnode = NULL;
> > +     hfs_find_result_init(fd);
> >       ptr = kzalloc(tree->max_key_len * 2 + 4, GFP_KERNEL);
> >       if (!ptr)
> >               return -ENOMEM;
> > @@ -106,17 +107,21 @@ int __hfs_brec_find(struct hfs_bnode *bnode,
> > struct hfs_find_data *fd,
> >       u16 off, len, keylen;
> >       int rec;
> >       int b, e;
> > -     int res;
> > +     int res = -ENOENT;
> >
> >       BUG_ON(!rec_found);
> > +     hfs_find_result_init(fd);
> > +     if (hfs_bnode_num_recs_invalid(bnode))
> > +             goto fail;
> > +
> >       b = 0;
> >       e = bnode->num_recs - 1;
> > -     res = -ENOENT;
> >       do {
> >               rec = (e + b) / 2;
> >               len = hfs_brec_lenoff(bnode, rec, &off);
> >               keylen = hfs_brec_keylen(bnode, rec);
> > -             if (keylen == 0) {
> > +             if (hfs_brec_len_invalid(bnode, len) ||
> > +                 hfs_brec_len_invalid(bnode, keylen)) {
> >                       res = -EINVAL;
> >                       goto fail;
> >               }
>
>
> 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.

Best Regards,
Jiaming Zhang