RE: [PATCH net v1] tipc: fix use-after-free of broadcast reception link at node release

[Tung Quang Nguyen]

>Subject: Re: [PATCH net v1] tipc: fix use-after-free of broadcast reception link at
>node release
>
>Thank you for your contribution! Sashiko AI review found 2 potential
>issue(s) to consider.
>
>Critical: 0 · High: 0 · Medium: 1 · Low: 1
>
>If you believe none of the issues are legitimate, please reply to each issue of
>Medium severity or higher and explain why the AI is wrong.
>If any of them are legitimate and you plan to respin, please let patchwork
>know by including "pw-bot: cr" as a separate line at the end of your reply (one
>such reply per series is enough).
>
>- [Medium] The commit message contradicts itself about runtime evidence.
>- [Low] The commit message says the issue was found with a Coccinelle
>  rule and 'LLM-assisted (GLM-5.3) code inspection', but the trailer…
>
>--
>
>tipc: fix use-after-free of broadcast reception link at node release
>
>This patch moves the kfree() of n->bc_entry.link out of
>tipc_node_kref_release() and into the tipc_node_free() RCU callback. As a
>result, the broadcast reception link is freed together with the enclosing
>tipc_node, after a grace period. Netlink walkers holding rcu_read_lock() can
>then no longer reach freed link memory.
>
>> Reproduced in a QEMU/KVM guest running v7.3.0 with KASAN and
>> PREEMPT(full): 10 TIPC peer namespaces on veth bearers, bearer removal
>> staggered so the 300 s node cleanup timers expire while other tasks
>> loop link dumps with the broadcast filter and reset-link-stats lookups.
>> KASAN hit within two cleanup cycles:
>>
>>   BUG: KASAN: slab-use-after-free in strlen+0x85/0xa0
>
>[ ... ]
>
>> with the freed object being the kmalloc-1k tipc_link allocated by
>> tipc_link_bc_create() during peer discovery.  The same defect fires in
>> strcmp() called from tipc_nl_node_reset_link_stats().
>
>[ ... ]
>
>> Found during a static-analysis-assisted audit: candidate sites were
>> surfaced by a Coccinelle rule, and the reader/writer paths were
>> confirmed by LLM-assisted (GLM-5.3) code inspection.  The issue was
>> not observed at runtime; it is identified by code inspection only.
>
>[Severity: Medium]
>These two paragraphs seem to contradict each other. Which one is accurate?
>
>The earlier paragraph describes a QEMU/KVM reproduction. It includes a
>KASAN slab-use-after-free splat from:
>
>  tipc_nl_node_dump_link()
>    __tipc_nl_add_node_links()
>      tipc_nl_add_bc_link()
>        strlen()
>
>with the object freed from tipc_node_timeout() in timer softirq context. It also
>says the same defect fires in strcmp() called from
>tipc_nl_node_reset_link_stats().
>
>The closing paragraph says the issue was not observed at runtime and was
>found by code inspection only.
>
>If the reproduction and the KASAN log were really captured, could the "not
>observed at runtime" sentence be dropped? If the issue was found by
>inspection only, could the reproduction paragraph and the splat be removed?
>

Agree. I think your fix is correct but the changelog needs to be rewritten.

>> Fixes: d25a01257e42 ("tipc: fix crash during node removal")
>> Signed-off-by: Binbin Deng <18983559317@xxxxxxx>
>
>[Severity: Low]
>This isn't a bug, but the message says a Coccinelle rule and GLM-5.3 were
>used, and there is no Assisted-by: trailer.
>
>Documentation/process/coding-assistants.rst asks for one in this format:
>
>  Assisted-by: LLM [TOOL1] [TOOL2]
>
>The "Using Assisted-by:" section of
>Documentation/process/submitting-patches.rst says:
>
>  If you used any sort of advanced coding tool in the creation of your
>  patch, you need to acknowledge that use by adding an Assisted-by tag.
>  Failure to do so may impede the acceptance of your work.
>
>Could a trailer such as "Assisted-by: GLM-5.3 coccinelle" be added?
>

Agree. Please add above tag.

--
pw-bot: cr