* RE: [PATCH net v2] tipc: reject name table updates with invalid origin node
2026-09-28 2:17 [PATCH net v2] tipc: reject name table updates with invalid origin node Jun Yang
@ 2026-09-29 11:07 ` Tung Quang Nguyen
2026-09-30 2:18 ` netdev-bot+sashiko
1 sibling, 0 replies; 3+ messages in thread
From: Tung Quang Nguyen @ 2026-09-29 11:07 UTC (permalink / raw)
To: Jun Yang
Cc: davem@davemloft.net, edumazet@kernel.org, kuba@kernel.org,
pabeni@redhat.com, horms@kernel.org, netdev@vger.kernel.org,
tipc-discussion@lists.sourceforge.net, jmaloy@redhat.com,
Jun Yang, stable@vger.kernel.org, TencentOS Corvus AI
>Subject: [PATCH net v2] tipc: reject name table updates with invalid origin node
>
>From: Jun Yang <junvyyang@tencent.com>
>
>tipc_rcv() locates the sending peer using msg_prevnode(), while
>tipc_named_rcv() takes the node address for NAME_DISTRIBUTOR updates
>from msg_orignode(). These are separate fields in the message header, so
>finding a valid peer does not validate the origin node.
>
>For a WITHDRAWAL with origin node 0, tipc_nametbl_remove_publ() treats
>the node as a wildcard and can remove a matching peer publication from the
>name table. tipc_node_unsubscribe() then returns without unlinking
>binding_node because in_own_node() treats node 0 as local.
>tipc_update_nametbl() subsequently calls kfree_rcu(), leaving its binding_node
>linked on the peer's publ_list.
>
>Once the publication has been freed, withdrawing an adjacent publication
>from the same peer can access the stale list entry in list_del_init().
>The following KASAN report shows this access in the list validation code:
>
> BUG: KASAN: slab-use-after-free in __list_del_entry_valid_or_report
> Read of size 8 at addr ffff8880249c7120 by task a.out/9456
>
> CPU: 0 UID: 0 PID: 9456 Comm: a.out Not tainted
> 7.3.0-rc4-00385-ga7bfaba4823e #81 PREEMPT(full)
>
> Call Trace:
> <IRQ>
> dump_stack_lvl lib/dump_stack.c:122
> print_address_description mm/kasan/report.c:379 [inline]
> print_report mm/kasan/report.c:482
> kasan_report mm/kasan/report.c:597
> __list_del_entry_valid_or_report lib/list_debug.c:62
> __list_del_entry include/linux/list.h:261 [inline]
> list_del_init include/linux/list.h:333 [inline]
> tipc_node_unsubscribe net/tipc/node.c:687
> tipc_update_nametbl net/tipc/name_distr.c:311 [inline]
> tipc_named_rcv net/tipc/name_distr.c:389
> tipc_rcv net/tipc/node.c:2202
> tipc_udp_recv net/tipc/udp_media.c:390
> udp_queue_rcv_one_skb net/ipv4/udp.c:2433
> udp_queue_rcv_skb net/ipv4/udp.c:2472
> udp_unicast_rcv_skb net/ipv4/udp.c:2625
> udp_rcv net/ipv4/udp.c:2697
>
>Reject zero and own-node origin addresses at the start of
>tipc_update_nametbl(), before inserting or removing any publication.
>Neither value is a valid origin for a peer name-table update, and
>in_own_node() covers both cases.
>
>Fixes: 218527fe27ad ("tipc: replace name table service range array with rb
>tree")
>Cc: stable@vger.kernel.org
>Reported-by: TencentOS Corvus AI <corvus@tencent.com>
>Assisted-by: tencentos-corvus-ai:hy4-preview
>Signed-off-by: Jun Yang <junvyyang@tencent.com>
>---
>v2:
>- Explain the distinction between the sending peer and the origin node.
>- Correct the Fixes tag to the commit introducing the node-zero wildcard.
>- Link to v1: https://lore.kernel.org/netdev/20260731101657.29119-1-
>juny24602@gmail.com/
>- Review:
>https://lore.kernel.org/netdev/GV1P189MB19887144315DA4A6AF16FE02C6D5
>2@GV1P189MB1988.EURP189.PROD.OUTLOOK.COM/
>
> net/tipc/name_distr.c | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/net/tipc/name_distr.c b/net/tipc/name_distr.c index
>ba4f4906e13b..a496e2e9ef62 100644
>--- a/net/tipc/name_distr.c
>+++ b/net/tipc/name_distr.c
>@@ -286,6 +286,9 @@ static bool tipc_update_nametbl(struct net *net, struct
>distr_item *i,
> u32 key = ntohl(i->key);
> struct tipc_uaddr ua;
>
>+ if (in_own_node(net, node))
This cannot happen as I explained already.
Please drop this patch.
pw-bot: rejected
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net v2] tipc: reject name table updates with invalid origin node
2026-09-28 2:17 [PATCH net v2] tipc: reject name table updates with invalid origin node Jun Yang
2026-09-29 11:07 ` Tung Quang Nguyen
@ 2026-09-30 2:18 ` netdev-bot+sashiko
1 sibling, 0 replies; 3+ messages in thread
From: netdev-bot+sashiko @ 2026-09-30 2:18 UTC (permalink / raw)
To: juny24602
Cc: tung.quang.nguyen, davem, edumazet, kuba, pabeni, horms, netdev,
tipc-discussion, jmaloy, junvyyang, stable, corvus
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 0 · Low: 0 (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).
- [High] The fix is incomplete: it does not check the origin node against
the peer that sent the message, so the same stale binding_node use-…
Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but the kernel-doc
for tipc_update_nametbl() in net/tipc/name_distr.c (lines 268-278)…
--
tipc: reject name table updates with invalid origin node
This patch adds an in_own_node() check at the start of
tipc_update_nametbl(). Name table updates whose origin node is 0 or the
local address are now dropped before any publication is inserted or
removed. The goal is to stop a stale binding_node from being left on a
peer's publ_list after kfree_rcu().
> diff --git a/net/tipc/name_distr.c b/net/tipc/name_distr.c
> index ba4f4906e13b7..a496e2e9ef623 100644
> --- a/net/tipc/name_distr.c
> +++ b/net/tipc/name_distr.c
> @@ -286,6 +286,9 @@ static bool tipc_update_nametbl(struct net *net, struct distr_item *i,
> u32 key = ntohl(i->key);
> struct tipc_uaddr ua;
>
> + if (in_own_node(net, node))
> + return false;
> +
[Severity: High]
Does this fully close the problem described in the commit message? The
message names the root cause as orignode never being checked against the
sending peer. This check only filters two special values, so any other
spoofed origin X still gets through.
In tipc_named_rcv() the origin still comes straight from the header:
node = msg_orignode(hdr);
...
tipc_update_nametbl(net, item, node, msg_type(hdr));
In tipc_rcv(), the peer n found through msg_prevnode() is not passed
down, so nothing compares node with n->addr:
tipc_named_rcv(net, &n->bc_entry.namedq,
&n->bc_entry.named_rcv_nxt,
&n->bc_entry.named_open);
Could a link peer A still reach the same list_del_init() use-after-free
through a node X that is known but not up? That could be a node A creates
with discovery frames, or a real node that went down recently. The
sequence would be:
1. A sends a PUBLICATION with origin X. tipc_node_subscribe() does not
check node_is_up(), so it links p->binding_node into X->publ_list:
tipc_node_write_lock(n);
list_add_tail(subscr, &n->publ_list);
2. After delete_at expires, tipc_node_timeout()->tipc_node_cleanup()
removes X:
if (!node_is_up(peer) && time_after(jiffies, peer->delete_at)) {
tipc_node_clear_links(peer);
tipc_node_delete_from_list(peer);
X is then freed through tipc_node_put()->tipc_node_kref_release()->
tipc_node_free(). Nothing on that path empties publ_list. publ_list
is only purged on NODE_DOWN via tipc_publ_notify(), which has either
already run or never runs for a node that was never up.
3. A recreates X with discovery, so tipc_node_create() allocates a new
node. A then sends a WITHDRAWAL with origin X and matching
type/lower/upper/port/key. tipc_node_unsubscribe() finds the new X and
runs:
list_del_init(subscr);
This goes through binding_node pointers that still point into the
freed tipc_node.
A spoofed origin for a node that does not exist at all also looks like a
problem. tipc_node_subscribe() only warns in that case, and the
publication is not purged when A goes down. That would let a remote peer
fill the name table with entries that are never cleaned up.
Would it be better to require msg_orignode() == n->addr for the node that
owns bc_entry.namedq? For example, n or its address could be passed into
tipc_named_rcv() from both callers in net/tipc/node.c. That check would
also cover the 0 and own-node cases, so it could replace this one rather
than sit next to it.
[Severity: Low]
This is a pre-existing issue that this patch didn't introduce. The
kernel-doc above tipc_update_nametbl() no longer matches the function:
* tipc_nametbl_lock must be held.
* Return: the publication item if successful, otherwise NULL.
The function is static bool and returns only true or false. There is no
tipc_nametbl_lock symbol. The caller tipc_named_rcv() actually holds
tn->nametbl_lock through spin_lock_bh().
This patch adds another return false path. Since the function is being
changed anyway, could the comment be updated at the same time? The only
caller ignores the return value, so there is no runtime impact.
> /* A peer-advertised binding with lower > upper can never be matched
> * or withdrawn and would leak the publication; the local bind path
> * rejects such ranges, so reject ranges learned from the network too.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928021807.7945-1-juny24602%40gmail.com
^ permalink raw reply [flat|nested] 3+ messages in thread