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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ec19e0e22401f2e372dde0aa81061401ebb4bedc.camel@mpiricsoftware.com>
Date: Tue, 03 Feb 2026 14:28:20 +0530
From: Shardul Bankar <shardul.b@...ricsoftware.com>
To: Viacheslav Dubeyko <Slava.Dubeyko@....com>, 
	"glaubitz@...sik.fu-berlin.de"
	 <glaubitz@...sik.fu-berlin.de>, "slava@...eyko.com" <slava@...eyko.com>, 
	"frank.li@...o.com"
	 <frank.li@...o.com>, "linux-kernel@...r.kernel.org"
	 <linux-kernel@...r.kernel.org>, "linux-fsdevel@...r.kernel.org"
	 <linux-fsdevel@...r.kernel.org>
Cc: "janak@...ricsoftware.com" <janak@...ricsoftware.com>, 
	"syzbot+1c8ff72d0cd8a50dfeaa@...kaller.appspotmail.com"
	 <syzbot+1c8ff72d0cd8a50dfeaa@...kaller.appspotmail.com>, 
	shardulsb08@...il.com
Subject: Re:  [PATCH v3] hfsplus: validate btree bitmap during mount and
 handle corruption gracefully

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