Re: [PATCH v3] hfsplus: validate btree bitmap during mount and handle corruption gracefully

[Shardul Bankar]

On Mon, 2026-02-02 at 20:52 +0000, Viacheslav Dubeyko wrote:
> On Sat, 2026-01-31 at 19:34 +0530, Shardul Bankar wrote:
> > 
> > +
> > +       /*
> > +        * Validate bitmap: node 0 (header node) must be marked
> > allocated.
> > +        */
> > +
> > +       node = hfs_bnode_find(tree, 0);
> 
> If you introduce named constant for herder node, then you don't need
> add this
> comment. And I don't like hardcoded value, anyway. :)

Hi Slava, thank you for the review.

Ack'ed. I will use HFSPLUS_TREE_HEAD (0) in v4.

> > +       len = hfs_brec_lenoff(node,
> > +                       HFSPLUS_BTREE_HDR_MAP_REC, &bitmap_off);
> > +
> > +       if (len != 0 && bitmap_off >= sizeof(struct
> > hfs_bnode_desc)) {
> 
> If we read incorrect len and/or bitmap_off, then it sounds like
> corruption too.
> We need to process this issue somehow but you ignore this, currently.
> ;)
> 

I agree that invalid offsets constitute corruption. However, properly
validating the structure of the record table and offsets is a larger
scope change. I prefer to keep this patch focused specifically on the
"unallocated node 0" vulnerability reported by syzbot. I am happy to
submit a follow-up patch to harden hfs_brec_lenoff usage. As per your
suggestion, ignoring this currently. ;)

> > +               hfs_bnode_read(node, &bitmap_byte, bitmap_off, 1);
> 
> I assume that 1 is the size of byte, then sizeof(u8) or
> sizeof(bitmap_byte)
> could look much better than hardcoded value.

Ack'ed. Changing to sizeof(bitmap_byte).

> 
> > +               if (!(bitmap_byte & HFSPLUS_BTREE_NODE0_BIT)) {
> 
> Why don't use the test_bit() [1] here? I believe that code will be
> more simple
> in such case.

I reviewed test_bit(), but I believe the explicit mask is safer and
more correct here for three reasons:
1. Endianness:
The value we’re checking is an on-disk bitmap byte (MSB of the first
byte in the header map record). test_bit() is designed for CPU-native
memory bitmaps. HFS+ bitmaps use Big-Endian bit ordering (Node 0 is the
MSB/0x80). On Little-Endian architectures (like x86), test_bit(0, ...)
checks the LSB (0x01). Using it here could introduce bit-numbering
ambiguity.

For example, reading into an unsigned long:
    unsigned long word = 0;
    hfs_bnode_read(node, &word, bitmap_off, sizeof(word));
    if (!test_bit(N, &word))
        ...

...but now N is not obviously “MSB of first on-disk byte”; it depends
on CPU endianness/bit numbering conventions, so it becomes easy to get
wrong.

2. Consistency with Existing HFS+ Bitmap Code:
The existing allocator code already handles on-disk bitmap bytes via
explicit masking (hfs_bmap_alloc uses 0x80, 0x40, ...), so for
consistency with existing on-disk bitmap handling and to avoid the
above ambiguity, I kept the explicit mask check here as well:
    if (!(bitmap_byte & HFSPLUS_BTREE_NODE0_BIT))   (with
HFSPLUS_BTREE_NODE0_BIT = BIT(7) (or (1 <<7)))

3. Buffer safety:
Reading exactly 1 byte (u8) guarantees we never read more data than
strictly required, avoiding potential boundary issues.

Am I missing something here or does this make sense?

If there's a strong preference for bitops helpers, I could investigate
the big-endian bit helpers (*_be), but for this single-byte invariant
check, the explicit mask seems clearest and most consistent with
existing code.

> 
> > +                       pr_warn("(%s): Btree 0x%x bitmap corruption
> > detected, forcing read-only.\n",
> 
> I prefer to mention what do we mean by 0x%x. Currently, it looks
> complicated to
> follow.

Ack'ed. I am adding a lookup to print the human-readable tree name
(Catalog, Extents, Attributes) alongside the ID.

> 
> >  #define HFSPLUS_ATTR_TREE_NODE_SIZE            8192
> >  #define HFSPLUS_BTREE_HDR_NODE_RECS_COUNT      3
> > +#define HFSPLUS_BTREE_HDR_MAP_REC              2       /* Map
> > (bitmap) record in header node */
> 
> Maybe, HFSPLUS_BTREE_HDR_MAP_REC_INDEX?

Ack'ed.

> 
> >  #define HFSPLUS_BTREE_HDR_USER_BYTES           128
> > +#define HFSPLUS_BTREE_NODE0_BIT                0x80
> 
> Maybe, (1 << something) instead of 0x80? I am OK with constant too.

Ack'ed, will use (1 << 7). Can also use BIT(7) if you prefer.

> Thanks,
Shardul