Re: [PATCH net 2/2] tipc: fix NULL pointer dereference in named_distribute()

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider.

Critical: 1 · High: 0 · Medium: 3 (2) · Low: 2

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

- [Critical] The fix is incomplete: a TIPC peer can still trigger the
  NULL dereference in named_distribute() (buf_msg(skb_peek_tail(list))…
- [Medium] The commit message lists "three major changes", all internal
  refactoring.
- [Medium] The new deferred-bulk flag n->dist_bulk can go stale and cause
  a second bulk distribution within one node session.
- [Medium] tipc: race in tipc_net_finalize() causes self-node publish to
  fail with -EEXIST
- [Low] The comment on the tn->nt_stop check in __tipc_named_distribute()
  says it protects against the name table having been deleted at…
- [Low] The new comment in tipc_service_delete() (net/tipc/name_table.c)
  says "tipc_nametbl_withdraw() does not delete 'p->binding_node' with…

Pre-existing issues:
- [Medium] tipc_named_reinit(), called from tipc_net_finalize(), sets
  nt->rc_dests = 0 unconditionally.
- [Medium] tipc_node_distribute() can leak SKBs, and the new timer path
  tipc_named_distribute() now reaches it too.

--

tipc: fix NULL pointer dereference in named_distribute()

This patch fixes a syzbot-reported NULL dereference in named_distribute().
The cause is a race between tipc_net_finalize() and the node-up bulk
distribution. The patch inserts the node's self-address TIPC_NODE_STATE
publication on demand from node-up or the node timer. It defers the bulk to
the node timer when the address is not set yet, and it adds error
propagation for allocation failures.

Closes: https://syzkaller.appspot.com/bug?extid=64197f7577a09686ffdb

> To facilitate the implementation, three major changes are made:
> - Move the sc->lock acquisition out of tipc_service_insert_publ() and into its
>   callers.
> - Add an output parameter (*err) to tipc_nametbl_publish() to detect memory
>   allocation errors.
> - Add three fields (nt_stop, nt_self_node_exist, and node_addr_set) to struct
>   tipc_net to synchronize the insertion and deletion of the node's self-address
>   publication.

[Severity: Medium]

These three items read as internal refactoring. The patch also changes
behaviour in several ways that the commit message does not mention. Should
these be described, or split into separate patches?

tipc_update_nametbl() now drops every peer PUBLICATION or WITHDRAWAL of
type TIPC_NODE_STATE whose range equals our address. It also logs "Can't
update duplicate node-up pub".

In tipc_node_write_unlock(), -ENOBUFS from tipc_named_node_up() now takes
down every link to the peer through the new tipc_node_down(). In the
parent commit ("tipc: fix self-deadlock in tipc_named_node_up") it only
called tipc_node_link_down(n, bearer_id, false).

An allocation failure while publishing the node-scope TIPC_LINK_STATE
binding on LINK_UP used to be ignored. It now forces that link down, which
seems unrelated to the named_distribute() race.

tipc_node_timeout() now forces every link to the peer down when the
deferred bulk redistribution returns -ENOBUFS.

Separately, can this new branch in tipc_nametbl_publish() ever run?

	if (!skb && p->scope == TIPC_CLUSTER_SCOPE && err)
		*err = -ENOBUFS;

The only caller that passes a non-NULL err is tipc_node_write_unlock(), and
it publishes TIPC_LINK_STATE with TIPC_NODE_SCOPE. tipc_net_finalize() and
tipc_sk_publish() both pass NULL.

> diff --git a/net/tipc/name_distr.c b/net/tipc/name_distr.c
> index a60f213ed1182..345bd767983e9 100644
> --- a/net/tipc/name_distr.c
> +++ b/net/tipc/name_distr.c

[ ... ]

> @@ -191,6 +197,94 @@ static void named_distribute(struct net *net, struct sk_buff_head *list,
>  	hdr = buf_msg(skb_peek_tail(list));
>  	msg_set_last_bulk(hdr);
>  	msg_set_named_seqno(hdr, seqno);
> +
> +	return 0;
> +}

[ ... ]

> +static int __tipc_named_distribute(struct net *net, u32 dnode,
> +				   struct sk_buff_head *head)
> +{
> +	struct name_table *nt = tipc_name_table(net);
> +	struct tipc_net *tn = tipc_net(net);
> +
> +	/* Name table has been deleted after namespace teardown or
> +	 * TIPC module exit.
> +	 */
> +	if (unlikely(tn->nt_stop))
> +		return 0;

[Severity: Low]

Can this check protect against a deleted name table? tipc_named_node_up()
already dereferences nt before __tipc_named_distribute() reads nt_stop:

	if (!(capabilities & TIPC_NAMED_BCAST))
		nt->rc_dests++;
	rc = __tipc_named_distribute(net, dnode, &head);

The scenario also does not look reachable. tipc_exit_net() runs
tipc_net_stop() first, which goes through
tipc_node_stop()->tipc_node_delete()->timer_delete_sync(). It then runs
cancel_work_sync(&tn->work), and only after that tipc_nametbl_stop().
tipc_nametbl_stop() sets nt_stop just before kfree_rcu(nt).

The only other caller of tipc_nametbl_stop() is the tipc_init_net() error
path, and no nodes exist at that point.

Is nt_stop needed at all? The commit message says it helps to "synchronize
the insertion and deletion of the node's self-address publication", which
does not seem accurate either.

> +
> +	/* Node's self-address is not set yet */
> +	if (!atomic_read(&tn->node_addr_set)) {
> +		/* Defer the distribution to node's timer */
> +		tipc_node_set_dist_bulk(net, dnode);
> +		return 0;
> +	}

[ ... ]

> +int tipc_named_distribute(struct net *net, u32 dnode)
> +{
> +	struct tipc_net *tn = tipc_net(net);
> +	struct sk_buff_head head;
> +	int bearer_id = 0;
> +	int rc = 0;
> +
> +	__skb_queue_head_init(&head);
> +	spin_lock_bh(&tn->nametbl_lock);
> +	rc = __tipc_named_distribute(net, dnode, &head);
> +	if (!rc && !skb_queue_empty(&head))
> +		rc = tipc_node_distribute(net, &head, dnode, &bearer_id, 0);

[Severity: Medium]

This is a pre-existing issue, not introduced by this patch, but the new
timer path through tipc_named_distribute() now reaches it as well. Can
tipc_node_distribute() leak skbs on its local xmitq?

When tipc_link_xmit() sends a multi-packet bulk, earlier packets may
already be cloned onto xmitq when a later skb_clone() fails:

	_skb = skb_clone(skb, GFP_ATOMIC);
	if (!_skb) {
		kfree_skb(skb);
		__skb_queue_purge(list);
		return -ENOBUFS;
	}

tipc_node_distribute() then skips the transmit and never purges xmitq:

	if (likely(rc != -ENOBUFS))
		tipc_bearer_xmit(net, *bearer_id, &xmitq, &maddr, n);

	tipc_node_put(n);

tipc_node_xmit() had the same pattern, and tipc_named_node_up() used it
before. tipc_node_distribute() was copied from it in the preceding commit
"tipc: fix self-deadlock in tipc_named_node_up".

> +	spin_unlock_bh(&tn->nametbl_lock);

[ ... ]

> @@ -426,5 +531,6 @@ void tipc_named_reinit(struct net *net)
>  	list_for_each_entry_rcu(p, &nt->cluster_scope, binding_node)
>  		p->sk.node = self;
>  	nt->rc_dests = 0;
> +	tn->nt_stop = false;
>  	spin_unlock_bh(&tn->nametbl_lock);
>  }

[Severity: Medium]

This is a pre-existing issue, not introduced by this patch. The patch now
explicitly supports node-up before finalize, though, so it seems relevant
here.

tipc_net_finalize() calls tipc_named_reinit(), which resets nt->rc_dests to
0 unconditionally. That wipes out the increment tipc_named_node_up() has
already made for any peer without TIPC_NAMED_BCAST that came up before
finalize:

tipc_named_node_up()
  nt->rc_dests++;
  __tipc_named_distribute()
    tipc_node_set_dist_bulk()     <- only the bulk is deferred
...
tipc_net_finalize()
  tipc_named_reinit()
    nt->rc_dests = 0;

Would later publications then see rc_dests == 0 and go out over the
broadcast link? Legacy peers do not accept name distribution that way.

When such a peer goes down, tipc_publ_notify() does nt->rc_dests--, which
wraps the u32 to 0xFFFFFFFF. tipc_node_broadcast() receives that as int -1
and stays in replicast from then on.

> diff --git a/net/tipc/name_table.c b/net/tipc/name_table.c
> index 8924bdb6a443f..3bcb8762bc9cc 100644
> --- a/net/tipc/name_table.c
> +++ b/net/tipc/name_table.c

[ ... ]

> +int tipc_nametb_insert_self_node_pub(struct net *net)
> +{

[ ... ]

> +	spin_lock_bh(&sc->lock);
> +	sr = tipc_service_find_range(sc, &ua);
> +	/* 1. tipc_net_finalize_work() is not scheduled because of namespace
> +	 *    teardown.
> +	 * 2. Or tipc_net_finalize() ---> tipc_nametbl_publish() has failed
> +	 *    to insert node self address publication into nt->cluster_scope
> +	 *    due to memory allocation failure.
> +	 * 3. Or tipc_net_finalize() ---> tipc_nametbl_publish() has not
> +	 *    executed yet.
> +	 */
> +	if (!sr) {

[ ... ]

> +exit:
> +	spin_unlock_bh(&sc->lock);
> +	return 0;
> +}

[Severity: Medium]
Is there a window between setting node_addr_set and the
tipc_nametbl_publish() call that follows it? node_addr_set is written
here without tn->nametbl_lock, but __tipc_named_distribute() reads it
under that lock. A node-up, or the node timer, can run in between. That
could be on another CPU, or in softirq on this one. It then does the
self-node insert first:

CPU0                          CPU1
tipc_net_finalize()
  atomic_set(node_addr_set)
                              tipc_named_node_up() or
                              tipc_node_timeout()
                                __tipc_named_distribute()
                                  tipc_nametb_insert_self_node_pub()
                                  tn->nt_self_node_exist = true;
                                  named_distribute()  <- dnode only
  tipc_nametbl_publish()
    tipc_nametbl_insert_publ()
      tipc_service_insert_publ()  <- duplicate, -EEXIST

tipc_nametb_insert_self_node_pub() builds the publication with the same
key, sk.ref and sk.node that tipc_net_finalize() passes. The duplicate
check in tipc_service_insert_publ() therefore rejects the second insert.
That prints the non-ratelimited "Failed to bind to" pr_warn() for what
is really an expected race. tipc_nametbl_publish() then returns NULL
without calling tipc_named_publish() or tipc_node_broadcast().

In that case the TIPC_NODE_STATE binding reaches only dnode, through
its bulk. Peers that are already up never get the broadcast that
finalize normally sends. They depend on their own deferred dist_bulk
being processed later. Is that intended?

The new comment above the call says the publication is redistributed
if tipc_nametbl_publish() fails. The -EEXIST case is not really a
failure, though, and it still warns.

Would it work to set node_addr_set only after tipc_nametbl_publish()
has returned? Alternatively, tipc_net_finalize() could insert the
publication under tn->nametbl_lock and set nt_self_node_exist there, so
both paths agree on which one owns the insert.

[Severity: Critical]

Can a [self,self] TIPC_NODE_STATE range that holds only a peer's entry make
this function return 0 without adding anything to nt->cluster_scope? The
function only checks that the range exists. It does not check that the
range holds this node's own publication, or that anything is linked on
nt->cluster_scope.

The new filter in tipc_update_nametbl() is skipped while tn->node_addr is
still 0:

	if (unlikely(type == TIPC_NODE_STATE && tn->node_addr &&
		     lower == tn->node_addr && upper == tn->node_addr)) {

During the address trial, tipc_disc_addr_trial_msg() schedules tn->work but
lets link setup continue while self == 0. The peer can see our trial
address, because we put it in prevnode in our discovery messages.
tipc_named_rcv() takes node = msg_orignode(hdr) without checking it against
the link peer.

So a peer could send an item like {type=TIPC_NODE_STATE,
lower=upper=trial_addr, port=0, key=trial_addr} with orignode=trial_addr.
orignode=0 would also work, because the !_p->sk.node test in
tipc_service_insert_publ() treats it as a wildcard. The sequence would then
be:

tipc_net_finalize()
  atomic_set(&tn->node_addr_set, 1);
  tipc_nametbl_publish(net, &ua, &sk, addr, NULL)
    tipc_service_insert_publ()   <- duplicate, -EEXIST, ignored

tipc_named_node_up() or tipc_node_timeout()->tipc_named_distribute()
  __tipc_named_distribute()
    tipc_nametb_insert_self_node_pub()   <- range found, returns 0
    tn->nt_self_node_exist = true;
    named_distribute(net, head, dnode, &nt->cluster_scope, ...)
      hdr = buf_msg(skb_peek_tail(list));   <- list is empty

With no cluster-scope user bindings, nt->cluster_scope is empty at this
point. The topology server binds with TIPC_NODE_SCOPE, so it does not add
anything. Would this hit the same NULL dereference in buf_msg() that the
patch sets out to fix, in softirq or timer context?

Even with an honest orignode, a planted [self,self] range still makes this
helper skip the insert. That reopens the window between
atomic_set(&tn->node_addr_set, 1) and tipc_nametbl_publish() in
tipc_net_finalize().

Would it be safer to check that our own publication is actually on
nt->cluster_scope? It might also help to have named_distribute() handle an
empty list, for example by returning early when skb_peek_tail(list) is
NULL.

[ ... ]

> @@ -940,6 +1043,15 @@ static void tipc_service_delete(struct net *net, struct tipc_service *sc)
>  	rbtree_postorder_for_each_entry_safe(sr, tmpr, &sc->ranges, tree_node) {
>  		list_for_each_entry_safe(p, tmp, &sr->all_publ, all_publ) {
>  			tipc_service_remove_publ(sr, &p->sk, p->key);
> +			/* tipc_nametbl_withdraw() does not delete
> +			 * 'p->binding_node' with type TIPC_NODE_STATE and
> +			 * range [tn->node_addr, tn->node_addr] from
> +			 * 'tn->cluster_scope'. So, delete it here.
> +			 */

[Severity: Low]

Is this comment accurate? tipc_nametbl_withdraw() calls
tipc_named_withdraw() for each publication it removes, and
tipc_named_withdraw() begins with an unconditional:

	list_del(&p->binding_node);

The self-node publication is still linked here only because nothing ever
withdraws it. tipc_net_stop() just calls tipc_bearer_stop() and
tipc_node_stop().

cluster_scope is also a member of struct name_table, not struct tipc_net,
so 'tn->cluster_scope' should probably read 'nt->cluster_scope'.

If a later change follows this comment and adds a withdraw call, would this
list_del() then remove the same entry twice?

> +			if (p->sr.type == TIPC_NODE_STATE &&
> +			    p->sr.lower == tn->node_addr &&
> +			    p->sr.upper == tn->node_addr)
> +				list_del(&p->binding_node);
>  			kfree_rcu(p, rcu);
>  		}

[ ... ]

> diff --git a/net/tipc/node.c b/net/tipc/node.c
> index b19d6652dc731..f566472a411fc 100644
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c

[ ... ]

> @@ -420,6 +430,8 @@ static void tipc_node_write_unlock(struct tipc_node *n)
>  	n->action_flags &= ~(TIPC_NOTIFY_NODE_DOWN | TIPC_NOTIFY_NODE_UP |
>  			     TIPC_NOTIFY_LINK_DOWN | TIPC_NOTIFY_LINK_UP);
>  
> +	if (flags & TIPC_NOTIFY_NODE_DOWN)
> +		n->dist_bulk = false;
>  	write_unlock_bh(&n->lock);

[ ... ]

> +void tipc_node_set_dist_bulk(struct net *net, u32 addr)
> +{
> +	struct tipc_node *node;
> +
> +	node = tipc_node_find(net, addr);
> +	if (node) {
> +		tipc_node_write_lock(node);
> +		node->dist_bulk = true;
> +		tipc_node_write_unlock_fast(node);
> +		tipc_node_put(node);
> +	}
> +}

[Severity: Medium]

Can n->dist_bulk go stale and trigger a second bulk within one node
session?

tipc_node_write_unlock() clears the flag on TIPC_NOTIFY_NODE_DOWN under
n->lock. tipc_named_node_up() runs only after that lock is dropped, and
tipc_node_set_dist_bulk() sets the flag without checking whether the node
has gone down in the meantime:

CPU0                                  CPU1
tipc_node_write_unlock() (NODE_UP)
  write_unlock_bh(&n->lock);
                                      link failure, NODE_DOWN
                                        n->dist_bulk = false;
  tipc_named_node_up()
    __tipc_named_distribute()
      node_addr_set == 0
      tipc_node_set_dist_bulk()
        node->dist_bulk = true;

Suppose the peer comes back after finalize has set node_addr_set. The new
NODE_UP sends a normal bulk right away. Later the node timer finds the
stale flag and sends a second one.

tipc_node_timeout() has a similar window. It reads and clears the flag
under n->lock, but calls tipc_named_distribute() after unlocking without
checking again.

On the receiver, tipc_named_dequeue() advances rcv_nxt to the extra bulk's
seqno and drops queued messages with a lower seqno:

	if (msg_is_last_bulk(hdr)) {
		*rcv_nxt = seqno;
		*open = true;
	}
	...
	if (less(seqno, *rcv_nxt)) {
		__skb_unlink(skb, namedq);
		kfree_skb(skb);

Could a withdrawal that has not been applied yet be lost this way, leaving
a stale binding on the peer? The duplicate items would also trigger the
non-ratelimited pr_warn() in tipc_service_insert_publ().

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028033722.125477-1-tung.quang.nguyen%40est.tech