Re: [syzbot] BUG: unable to handle kernel paging request in __hfsplus_brec_find

[syzbot <syzbot+f957cf00261d1c41fafd@syzkaller.appspotmail.com>]

> Hi,
>
> I am investigating the HFS+ crash reported by syzbot in
> __hfsplus_brec_find() and would like to share the current results of a
> function-by-function reconstruction of the failure path.
>
> The public report shows the fault address:
>
> fffffffffffffffb
>
> On 64-bit Linux this value is consistent with the encoding of
> ERR_PTR(-EIO). I am treating that correspondence as an important clue,
> not as proof that -EIO originated at any particular call site.
>
> STATIC AUDIT
>
> The audit identified two concrete producer-to-consumer gaps in
> fs/hfsplus/brec.c. In both cases, hfs_bnode_find() can supply a value
> that is assigned to fd->bnode without an IS_ERR() check before
> fd->bnode is subsequently consumed as a struct hfs_bnode pointer.
>
> The first site is in hfs_brec_insert(), after a successful node split
> and during the parent-node lookup. A second related site exists in
> hfs_brec_update_parent().
>
> hfs_bnode_find() has error-return paths using ERR_PTR(), including
> -EIO. This makes error-pointer propagation through these unchecked
> assignments a mechanism worth testing. However, the existence of these
> paths alone does not establish that either site produced the error
> pointer in the original syzbot execution.
>
> RUNTIME REACHABILITY
>
> I then tested the relevant control flow in an isolated QEMU HFS+ environment.
>
> A clean HFS+ filesystem and a workload creating many long catalog
> names naturally caused a Catalog B-tree node split and reached the
> parent lookup in hfs_brec_insert(). No control-flow manipulation was
> needed to reach that branch.
>
> This established runtime reachability of the exact branch containing
> the first unchecked hfs_bnode_find() assignment.
>
> DIRECTED ERROR-POINTER EXPERIMENT
>
> Only after the natural control flow reached that parent-lookup point,
> I deliberately injected:
>
> fd->bnode = ERR_PTR(-EIO)
>
> The unprotected execution then faulted at:
>
> fffffffffffffffb
>
> This is the same numerical fault address shown in the public syzbot report.
>
> I want to be explicit about the interpretation of this experiment: the
> ERR_PTR(-EIO) value in this test was deliberately injected. Therefore
> this is NOT a natural reproduction of the syzbot bug and does NOT
> demonstrate that hfs_bnode_find() naturally returned -EIO in the
> original report.
>
> What the experiment demonstrates is narrower: if ERR_PTR(-EIO) reaches
> fd->bnode at this reachable producer-to-consumer gap, the resulting
> invalid pointer can produce the same fault-address value observed by
> syzbot.
>
> NAIVE CONTAINMENT AND CLEANUP BEHAVIOR
>
> An initial diagnostic attempt added an IS_ERR() check after assigning
> hfs_bnode_find() directly to fd->bnode and returned the corresponding
> error.
>
> That guard detected the injected -EIO, but the kernel subsequently
> faulted again at fffffffffffffffb, this time through the cleanup path
> ending in hfs_bnode_put().
>
> The reason was that fd->bnode still retained ERR_PTR(-EIO). The caller
> cleanup eventually executed hfs_find_exit(), which calls
> hfs_bnode_put(fd->bnode) without treating ERR_PTR as a valid state.
>
> This led to an additional source-level observation: fd->bnode appears
> to have an effective cleanup contract of containing either a valid
> hfs_bnode pointer or NULL, not an ERR_PTR value. Existing HFS+ code
> also contains a safe pattern in which the hfs_bnode_find() result is
> first held in a temporary pointer, checked with IS_ERR(), and only
> then assigned to fd->bnode.
>
> NEW_NODE OWNERSHIP
>
> The split path also has a live new_node reference returned by
> hfs_bnode_split(). Therefore simply returning on a failed parent
> lookup would not be sufficient; the diagnostic error exit must also
> account for that reference.
>
> DIAGNOSTIC PATCH 40P
>
> Based on those observations, I developed 40P as a diagnostic
> containment patch. It modifies the two unchecked hfs_bnode_find()
> sites identified in the audit.
>
> At each site, the hfs_bnode_find() result is first stored in a
> temporary pointer. If IS_ERR() is true, the patch:
>
> 1. obtains the error with PTR_ERR();
> 2. ensures fd->bnode is NULL rather than retaining the error pointer;
> 3. releases the live new_node reference with hfs_bnode_put(new_node);
> 4. returns the error;
> 5. assigns the temporary pointer to fd->bnode only after it has passed
> the error check.
>
> Under the same directed ERR_PTR(-EIO) condition, this diagnostic
> version contained the tested error-pointer propagation without leaving
> fd->bnode poisoned for the later cleanup path.
>
> 40P is a diagnostic patch only. It is not intended as an upstream fix.
> Its purpose is to test and constrain the causal path while preserving
> the local cleanup state observed in the source.
>
> CURRENT EVIDENCE BOUNDARY
>
> The evidence currently supports the following statements:
>
> - The public syzbot report contains the fault address fffffffffffffffb.
> - That value is consistent with ERR_PTR(-EIO).
> - Two unchecked hfs_bnode_find() -> fd->bnode producer-to-consumer
> gaps were identified in brec.c.
> - hfs_bnode_find() can return error pointers, including -EIO on an error path.
> - hfs_brec_insert() and the relevant post-split parent-lookup branch
> are naturally runtime-reachable in the controlled HFS+ workload.
> - A deliberately injected ERR_PTR(-EIO) at that reachable point
> produces the same numerical fault-address value.
> - A naive IS_ERR()+return is insufficient because fd->bnode can remain
> poisoned and be consumed during cleanup.
> - 40P contains that directed condition while maintaining fd->bnode as
> NULL on the tested error exit and releasing new_node.
>
> The evidence does NOT yet establish:
>
> - that the original syzbot execution naturally obtained -EIO from
> hfs_bnode_find() at this site;
> - that either of these two unchecked assignments is the complete root
> cause of the public crash;
> - or where the first naturally occurring invalid/error state
> originates in the syzbot execution.
>
> NEXT DIAGNOSTIC QUESTION
>
> I am continuing to move the observation point backward around the
> Catalog B-tree split and parent lookup, with the goal of separating:
>
> 1. the point where an error pointer can be prevented from propagating; and
> 2. the point where the relevant error state first arises naturally.
>
> I would appreciate feedback on whether these two hfs_bnode_find()
> sites are the appropriate boundary for continued instrumentation, and
> whether there are specific HFS+ B-tree invariants, parent-node state
> transitions, or error-propagation rules around
> hfs_bnode_split()/parent lookup that should be checked next.
>
> #syz test

This crash does not have a reproducer. I cannot test it.

>
> The 40P diagnostic patch is attached as plain text so its whitespace
> is preserved reliably.
>
> Thanks,
> David Maximiliano Hermitte