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

[Shardul Bankar]

On Tue, 2026-02-03 at 23:12 +0000, Viacheslav Dubeyko wrote:
> 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:
> > > 
> > > > 
> > > > +       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 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.

Hi Slava,

Ah, I misunderstood you previously!

You are right. I will add an explicit error log and force SB_RDONLY for
this invalid offset/length case in v4 as well.

> 
> > 
> > > > +               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.
> > 
> > 
> 
> 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?
> :)

You are right that a single byte's representation is the same in memory
regardless of CPU endianness. To be completely transparent, the precise
interplay between cross-architecture kernel bitops, memory alignment,
and on-disk format endianness has always been a bit of a conceptual
blind spot for me.

My initial hesitation with `test_bit()` was also that it strictly
requires an `unsigned long *`. Reading a 1-byte on-disk requirement
directly into an `unsigned long` buffer felt sub-optimal and could risk
out-of-bounds stack reads if not handled carefully.

However, I want to follow your recommendation to standardize on kernel
bitops. To safely use `test_bit()`, I propose reading the byte safely
into a `u8`, and then promoting it to an `unsigned long` before
testing; something like this on top of the patch:

diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c
index 4bd0d18ac6c6..7ce1708e423c 100644
--- a/fs/hfsplus/btree.c
+++ b/fs/hfsplus/btree.c
@@ -135,6 +135,7 @@ struct hfs_btree *hfs_btree_open(struct super_block
*sb, u32 id)
        struct hfs_btree *tree;
        struct hfs_btree_header_rec *head;
        struct address_space *mapping;
+       unsigned long bitmap_word;
        struct hfs_bnode *node;
        u16 len, bitmap_off;
        struct inode *inode;
@@ -255,7 +256,8 @@ struct hfs_btree *hfs_btree_open(struct super_block
*sb, u32 id)
 
        if (len != 0 && bitmap_off >= sizeof(struct hfs_bnode_desc)) {
                hfs_bnode_read(node, &bitmap_byte, bitmap_off,
sizeof(bitmap_byte));
-               if (!(bitmap_byte & HFSPLUS_BTREE_NODE0_BIT)) {
+               bitmap_word = bitmap_byte;
+               if (!test_bit(HFSPLUS_BTREE_NODE0_BIT, &bitmap_word)) {
                        const char *tree_name;
 
                        switch (id) {
diff --git a/include/linux/hfs_common.h b/include/linux/hfs_common.h
index f2f70975727c..52e9b8969406 100644
--- a/include/linux/hfs_common.h
+++ b/include/linux/hfs_common.h
@@ -512,7 +512,7 @@ struct hfs_btree_header_rec {
 #define HFSPLUS_BTREE_HDR_NODE_RECS_COUNT      3
 #define HFSPLUS_BTREE_HDR_MAP_REC_INDEX        2       /* Map (bitmap)
record in header node */
 #define HFSPLUS_BTREE_HDR_USER_BYTES           128
-#define HFSPLUS_BTREE_NODE0_BIT                0x80
+#define HFSPLUS_BTREE_NODE0_BIT                7
 
 /* btree key type */
 #define HFSPLUS_KEY_CASEFOLDING                0xCF    /* case-
insensitive */


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

According to Apple's Tech Note TN1150 [1], HFS+ maps Node 0 to the Most
Significant Bit (MSB).

Specifically, TN1150 states:

1. Under "Map Record": "The bits are interpreted in the same way as the
bits in the allocation file."
2. Under "Allocation File": "Within each byte, the most significant bit
holds information about the allocation block with the lowest number..."

Apple also provides a C code snippet in the document (Listing 1)
demonstrating this:
`return (thisByte & (1 << (7 - (thisAllocationBlock % 8)))) != 0;`

If we evaluate this for Node 0 (`thisAllocationBlock = 0`), it resolves
to `1 << 7` (or bit 7). Therefore, checking bit 7 is mathematically
required to target Node 0.

If these changes look proper to you, I will add them to my code along
with the rest of your feedback and send out v4 shortly.

Thanks again for the guidance, continued review and for bearing with me
on this!

Regards,
Shardul

[1] Apple Tech Note TN1150:
https://developer.apple.com/library/archive/technotes/tn/tn1150.html#AllocationFile