Message-ID: <c755dddccae01155eb2aa72d6935a4db939d2cd7.camel@ibm.com>
Date: Tue, 3 Feb 2026 23:12:41 +0000
From: Viacheslav Dubeyko <Slava.Dubeyko@....com>
To: "shardul.b@...ricsoftware.com" <shardul.b@...ricsoftware.com>,
        "glaubitz@...sik.fu-berlin.de" <glaubitz@...sik.fu-berlin.de>,
        "frank.li@...o.com" <frank.li@...o.com>,
        "slava@...eyko.com"
	<slava@...eyko.com>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        "linux-fsdevel@...r.kernel.org"
	<linux-fsdevel@...r.kernel.org>
CC: "syzbot+1c8ff72d0cd8a50dfeaa@...kaller.appspotmail.com"
	<syzbot+1c8ff72d0cd8a50dfeaa@...kaller.appspotmail.com>,
        "janak@...ricsoftware.com" <janak@...ricsoftware.com>,
        "shardulsb08@...il.com" <shardulsb08@...il.com>
Subject: RE:  [PATCH v3] hfsplus: validate btree bitmap during mount and
 handle corruption gracefully

On Tue, 2026-02-03 at 14:28 +0530, Shardul Bankar wrote:
> 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. ;)

I am not suggesting to check everything. But because you are using these values
and you can detect that these value is incorrect, then it makes sense to report
the error if something is wrong.

> 
> > > +               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?

Correct me if I am wrong, but I suspect that I am right. :) The endianness is
about bytes not bits. I am googled this: "Endianness defines the order in which
bytes, constituting multibyte data (like 16, 32, or 64-bit integers), are stored
in memory or transmitted over a network.". So, if you need manage endianness of
of values, then you can use cpu_to_bexx()/bexx_to_cpu() family of methods. But
it's not about bits. If you take byte only, then the representation of byte is
the same in Big-Endian (BE) or Little-Endian (LE). Am I right here? :)

> 
> 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.

OK. So, are you sure that node #0 corresponds to bit #7 but not bit #0? :) I am
started to doubt that we are correct here.

Thanks,
Slava.

Powered by blists - more mailing lists