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

[ThangNN99]

Hi Slava,

Thanks for looking at this.

> Could you please explain the use-case or workload that is trying to
> claim more blocks that fork can include for Extents Overflow file?

It is not a normal workload -- it requires a corrupted/adversarial
on-disk volume, e.g. a loop-mounted image (removable media, a
downloaded .img/.dmg, or a fuzzer). Per Apple's TN1150 ("HFS Plus
Volume Format"):

  "The extents overflow file also stores additional extents for the
  special files except for the extents overflow file itself."

So by design the extents overflow file must always be fully described
by the eight extents in its own fork record; it can never legitimately
need an overflow extent of its own. The syzbot reproducer mounts an
image whose volume header sets the Extents File fork's total block
count higher than what its eight direct extents describe, which puts
hip->alloc_blocks != hip->first_blocks for HFSPLUS_EXT_CNID -- a state
the volume header alone can force without the extents tree itself
being touched. hfs_btree_open() doesn't currently validate this fork
against the invariant above.

Once mounted, a plain pwritev2() to a regular file (call it FILE_A)
that already has extents cached from a previous lookup is enough to
hit it:

> Could you please share the call trace for the issue?

  pwritev2
   -> hfsplus_get_block(FILE_A)
   -> hfsplus_file_extend(FILE_A)
   -> hfsplus_ext_read_extent(FILE_A) -> hfs_find_init(ext_tree)   [tree_lock acquired]
   -> __hfsplus_ext_cache_extent(FILE_A): FILE_A's cached extent is dirty
   -> __hfsplus_ext_write_extent(FILE_A): needs to insert a new record
   -> hfs_bmap_reserve(ext_tree): ext_tree itself is out of free nodes
   -> hfsplus_file_extend(ext_tree->inode)          <- now growing the tree's own file
   -> hfsplus_ext_read_extent(ext_tree->inode) -> hfs_find_init(ext_tree)  [tree_lock again -> deadlock]

Full syzbot lockdep report for reference:

WARNING: possible recursive locking detected
6.16.0-rc7-syzkaller-00120-g5f33ebd2018c #0 Not tainted
--------------------------------------------
syz-executor310/5840 is trying to acquire lock:
ffff88807fe920b0 (&tree->tree_lock/1){+.+.}-{4:4}, at: hfsplus_find_init+0x15a/0x1d0 fs/hfsplus/bfind.c:28

but task is already holding lock:
ffff88807fe920b0 (&tree->tree_lock/1){+.+.}-{4:4}, at: hfsplus_find_init+0x15a/0x1d0 fs/hfsplus/bfind.c:28

5 locks held by syz-executor310/5840:
 #0: sb_writers#8, at: vfs_writev+0x288/0x960 fs/read_write.c:1055
 #1: &sb->s_type->i_mutex_key#14, at: generic_file_write_iter+0xe3/0x540 mm/filemap.c:4252
 #2: &hip->extents_lock, at: hfsplus_file_extend+0x1fc/0x1990 fs/hfsplus/extents.c:458
 #3: &tree->tree_lock/1, at: hfsplus_find_init+0x15a/0x1d0 fs/hfsplus/bfind.c:28
 #4: &HFSPLUS_I(inode)->extents_lock, at: hfsplus_file_extend+0x1fc/0x1990 fs/hfsplus/extents.c:458

Call Trace:
 hfsplus_find_init fs/hfsplus/bfind.c:28
 hfsplus_ext_read_extent fs/hfsplus/extents.c:216 [inline]
 hfsplus_file_extend+0x416/0x1990 fs/hfsplus/extents.c:462
 hfsplus_bmap_reserve+0x122/0x500 fs/hfsplus/btree.c:358
 __hfsplus_ext_write_extent+0x28d/0x5b0 fs/hfsplus/extents.c:104
 __hfsplus_ext_cache_extent+0x89/0xe30 fs/hfsplus/extents.c:186
 hfsplus_ext_read_extent fs/hfsplus/extents.c:218 [inline]
 hfsplus_file_extend+0x444/0x1990 fs/hfsplus/extents.c:462
 hfsplus_get_block+0x411/0x1530 fs/hfsplus/extents.c:245
 __block_write_begin_int+0x6b2/0x1900 fs/buffer.c:2151
 block_write_begin fs/buffer.c:2262 [inline]
 cont_write_begin+0x789/0xb50 fs/buffer.c:2601
 hfsplus_write_begin+0x66/0xb0 fs/hfsplus/inode.c:46
 generic_perform_write+0x2c4/0x910 mm/filemap.c:4112
 generic_file_write_iter+0x10f/0x540 mm/filemap.c:4255
 do_iter_readv_writev+0x56b/0x7f0 fs/read_write.c:-1
 vfs_writev+0x31a/0x960 fs/read_write.c:1057
 do_pwritev fs/read_write.c:1153 [inline]
 __se_sys_pwritev2+0x179/0x290 fs/read_write.c:1202

(full report: https://syzkaller.appspot.com/text?tag=CrashReport&x=172748a2580000)

hfsplus_get_block() already refuses HFSPLUS_EXT_CNID for the lookup
direction (extents.c:261: "if (inode->i_ino == HFSPLUS_EXT_CNID)
return -EIO;"). This patch adds the same refusal on the grow path in
hfsplus_file_extend(), which is the one hfs_bmap_reserve() can reach
with tree_lock already held. It doesn't fix the underlying corruption,
just stops it from self-deadlocking the tree_lock; a hfs_btree_open()
check that rejects such a volume outright at mount time would close
the hole earlier and I'm happy to send that as a follow-up if you'd
rather validate it there instead.

I have not personally observed the crash on current mainline: I
rebuilt the syzbot C reproducer, and hfs_btree_open(HFSPLUS_CAT_CNID)
now rejects the image at mount (silently) -- it's a 2022-era fuzzed
image and mainline has since gained catalog b-tree validation
(node-size sanity check, record-offset table validation, etc.) that
this image no longer passes. The analysis above is derived from source
plus the syzbot-provided trace, not from a reproduced local crash.

Thanks,
Thang