Re: [PATCH] hfsplus: fix recursive tree_lock in hfsplus_file_extend()

[ThangNN99]

Hi Slava,

> Could you share the dump/content of the Extents Overflow file's fork?

Decoded the volume header from the syzbot image. The Extents Overflow
fork (offset 192 in the header):

  logicalSize=32768 clumpSize=32768 totalBlocks=32
  extents[0]=(start=3,   count=32)
  extents[1]=(start=0,   count=0)
  extents[2]=(start=0,   count=0)
  extents[3]=(start=0,   count=134217728)   <- garbage
  extents[4]=(start=0,   count=0)
  extents[5]=(start=0,   count=0)
  extents[6]=(start=0,   count=11796736)    <- garbage
  extents[7]=(start=0,   count=0)

Correction to my last mail: it's not totalBlocks exceeding the fork's
extents, it's the reverse and messier. hfsplus_inode_read_fork() sums
all 8 extents' block_count into hip->first_blocks with no validation
(inode.c:569). Slots 3 and 6 have start_block=0 (i.e. "unused") but
garbage non-zero block_count, so first_blocks comes out to 146014496
against a real totalBlocks (hip->alloc_blocks) of 32. Either direction
of that mismatch takes hfsplus_file_extend() down the same
hfsplus_ext_read_extent() path, since the code only tests
alloc_blocks == first_blocks.

> Could we detect the corruption of the fork during the mount phase?
> If we can then we need to mount in Read-Only mode the corrupted
> volume.

Yes. Proposed v2, forcing read-only instead of touching extents.c:

--- a/fs/hfsplus/btree.c
+++ b/fs/hfsplus/btree.c
@@ -293,6 +293,14 @@ struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id)
 		goto free_inode;
 	}

+	/* Per TN1150, the extents file can't have overflow extents of its own. */
+	if (id == HFSPLUS_EXT_CNID &&
+	    HFSPLUS_I(tree->inode)->first_blocks !=
+	    HFSPLUS_I(tree->inode)->alloc_blocks) {
+		pr_warn("extents overflow file has overflow extents of its own, forcing read-only.\n");
+		sb->s_flags |= SB_RDONLY;
+	}
+
 	mapping = tree->inode->i_mapping;
 	page = read_mapping_page(mapping, 0, NULL);
 	if (IS_ERR(page))

One catch: the reproducer mounts MS_RDONLY, then remounts rw via a
bare MS_REMOUNT|MS_MOVE. hfsplus_reconfigure() only re-checks
VOL_UNMNT/SOFTLOCK/JOURNALED before allowing that, and this image sets
VOL_UNMNT, so a read-only-only fix in hfs_btree_open() gets undone by
that remount. Same check needs to go in hfsplus_reconfigure() too:

--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -400,6 +400,12 @@ static int hfsplus_reconfigure(struct fs_context *fc)
 			pr_warn("filesystem is marked journaled, leaving read-only.\n");
 			sb->s_flags |= SB_RDONLY;
 			fc->sb_flags |= SB_RDONLY;
+		} else if (HFSPLUS_I(sbi->ext_tree->inode)->first_blocks !=
+				HFSPLUS_I(sbi->ext_tree->inode)->alloc_blocks) {
+			/* Per TN1150, the extents file can't have overflow extents of its own. */
+			pr_warn("extents overflow file has overflow extents of its own, leaving read-only.\n");
+			sb->s_flags |= SB_RDONLY;
+			fc->sb_flags |= SB_RDONLY;
 		}
 	}
 	return 0;

Both hunks build cleanly here. Want me to send this as v2 replacing
the extents.c hunk, or keep the extents.c guard too as a second line
of defense (it's independent of mount-time state and free)?

Thanks,
Thang