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

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

[Binbin Deng]

When the last reference to a tipc_node is dropped, the node itself is
deferred to the RCU callback tipc_node_free(), but the nested broadcast
reception link, n->bc_entry.link, is kfree()d synchronously in
tipc_node_kref_release(), before call_rcu(), and the pointer is never
cleared.  Readers that obtained the node from node_list and then
dereference bc_entry.link deep into the link (tipc_link_name() in
tipc_nl_node_reset_link_stats(), bcl->name/stats in
tipc_nl_add_bc_link() on the link dump path) therefore touch freed
memory although the enclosing node stays alive for a full grace period.
The sibling per-bearer links get "kfree + NULL" under the node write
lock in tipc_node_clear_links(); the broadcast entry was missed.

The race is reachable through the node cleanup timer: once a peer has
been unreachable for NODE_CLEANUP_AFTER (300 s), tipc_node_timeout() ->
tipc_node_cleanup() unlinks the node under the write lock, and the
final tipc_node_put() at the caller runs outside node_list_lock, so the
kfree executes in timer softirq context, concurrently with genl dump/
reset walkers that hold only rcu_read_lock().  (A netlink "peer remove"
cannot race those walkers itself: tipc_genl_family has no parallel_ops,
so all doit/dumpit calls serialize on the global genl lock.)

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
  Read of size 1 at addr ffff88800b54a004 by task poc_eth/371
   strlen+0x85/0xa0
   tipc_nl_add_bc_link+0x2f9/0xd40
   __tipc_nl_add_node_links+0x10c/0x140
   tipc_nl_node_dump_link+0x8d5/0xd50
  Freed by task 0:
   kfree+0x121/0x380
   tipc_node_timeout+0xa33/0xcf0
   run_timer_softirq+0x144/0x250
   sysvec_apic_timer_interrupt

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().

Move the kfree of n->bc_entry.link into the tipc_node_free() RCU
callback so the broadcast link lives exactly as long as the enclosing
node.  All readers are covered: the netlink walkers are inside
rcu_read_lock(), and the packet reception path reaches the node via
tipc_node_find(), whose kref defers kref_release itself.

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.

Fixes: d25a01257e42 ("tipc: fix crash during node removal")
Signed-off-by: Binbin Deng <18983559317@163.com>
---
 net/tipc/node.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/tipc/node.c b/net/tipc/node.c
index bd91378b7540..fdc679fe2d91 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -303,6 +303,7 @@ static void tipc_node_free(struct rcu_head *rp)
 #ifdef CONFIG_TIPC_CRYPTO
 	tipc_crypto_stop(&n->crypto_rx);
 #endif
+	kfree(n->bc_entry.link);
 	kfree(n);
 }
 
@@ -310,7 +311,6 @@ static void tipc_node_kref_release(struct kref *kref)
 {
 	struct tipc_node *n = container_of(kref, struct tipc_node, kref);
 
-	kfree(n->bc_entry.link);
 	call_rcu(&n->rcu, tipc_node_free);
 }
 
-- 
2.43.0

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

[netdev-bot+sashiko]

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?

> Fixes: d25a01257e42 ("tipc: fix crash during node removal")
> Signed-off-by: Binbin Deng <18983559317@163.com>

[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?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015041435.33561-1-18983559317%40163.com

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@163.com>
>
>[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