[PATCH] tipc: fix use-after-free in netns teardown

[PATCH] tipc: fix use-after-free in netns teardown

[Jérémy Jean]

conn_put() decreases the reference count of a connection, and when it
reaches zero, tipc_conn_kref_release() removes the connection from
conn_idr and frees it. Removing the entry is protected by idr_lock,
but decreasing the count is not.

During netns dismantle, it may happen that a connection gets its count
dropped to zero by conn_put() while tipc_topsrv_stop() still holds the
lock idr_lock. tipc_topsrv_stop() calls conn_get() on that connection
whose count is already zero, and then releases idr_lock.
tipc_conn_kref_release() can then free the connection before
tipc_conn_close() uses it. KASAN reports the UAF as:

  BUG: KASAN: slab-use-after-free in tipc_conn_close (net/tipc/topsrv.c:158)
  Read of size 8 at addr ff1100000cf4f408 by task kworker/u16:3/70

Stop accepting connections before walking conn_idr. Use
kref_get_unless_zero() to avoid taking references when the count is zero.
Retry from the first entry, releasing idr_lock and calling cond_resched()
between iterations so pending work can finish.

Fixes: 667eeab4999e ("tipc: Fix use-after-free in tipc_conn_close().")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr>
---
 net/tipc/topsrv.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
index af530c9ed8402..9ab2349f71190 100644
--- a/net/tipc/topsrv.c
+++ b/net/tipc/topsrv.c
@@ -700,16 +700,23 @@ static void tipc_topsrv_stop(struct net *net)
 	struct tipc_conn *con;
 	int id;
 
+	write_lock_bh(&lsock->sk->sk_callback_lock);
+	lsock->sk->sk_user_data = NULL;
+	write_unlock_bh(&lsock->sk->sk_callback_lock);
+	cancel_work_sync(&srv->awork);
+
 	spin_lock_bh(&srv->idr_lock);
-	for (id = 0; srv->idr_in_use; id++) {
-		con = idr_find(&srv->conn_idr, id);
+	for (id = 0; srv->idr_in_use; id = 0) {
+		con = idr_get_next(&srv->conn_idr, &id);
+		if (con && !kref_get_unless_zero(&con->kref))
+			con = NULL;
+		spin_unlock_bh(&srv->idr_lock);
 		if (con) {
-			conn_get(con);
-			spin_unlock_bh(&srv->idr_lock);
 			tipc_conn_close(con);
 			conn_put(con);
-			spin_lock_bh(&srv->idr_lock);
 		}
+		cond_resched();
+		spin_lock_bh(&srv->idr_lock);
 	}
 	__module_get(lsock->ops->owner);
 	__module_get(lsock->sk->sk_prot_creator->owner);
-- 
2.47.3

Re: [PATCH] tipc: fix use-after-free in netns teardown

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH] tipc: fix use-after-free in netns teardown

[netdev-bot+sashiko]

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

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

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] Because the teardown loop in tipc_topsrv_stop() always
  restarts at the lowest IDR id (line 709, `id = 0`), it can block on…

Pre-existing issues:
- [High] Netns teardown can hit a NULL pointer dereference on a kernel-
  subscription connection.

--

tipc: fix use-after-free in netns teardown

Before walking conn_idr, this patch makes tipc_topsrv_stop() clear the
listener's sk_user_data and cancel the accept work. The idr_find() plus
conn_get() lookup is replaced with idr_get_next() plus
kref_get_unless_zero(). The walk now restarts from id 0 on every pass and
calls cond_resched() between passes.

> diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
> index af530c9ed8402..9ab2349f71190 100644
> --- a/net/tipc/topsrv.c
> +++ b/net/tipc/topsrv.c
> @@ -700,16 +700,23 @@ static void tipc_topsrv_stop(struct net *net)
>  	struct tipc_conn *con;
>  	int id;
>  
> +	write_lock_bh(&lsock->sk->sk_callback_lock);
> +	lsock->sk->sk_user_data = NULL;
> +	write_unlock_bh(&lsock->sk->sk_callback_lock);
> +	cancel_work_sync(&srv->awork);
> +
>  	spin_lock_bh(&srv->idr_lock);
> -	for (id = 0; srv->idr_in_use; id++) {
> -		con = idr_find(&srv->conn_idr, id);
> +	for (id = 0; srv->idr_in_use; id = 0) {
> +		con = idr_get_next(&srv->conn_idr, &id);

[Severity: Medium]

Can restarting from id 0 on every pass livelock the teardown?

rcv_wq is an ordered workqueue:

tipc_topsrv_work_start()
    s->rcv_wq = alloc_ordered_workqueue("tipc_rcv", 0);

Take a lower-id connection A whose rwork is queued behind the running
rwork of a higher-id connection B. The loop closes A, but the reference
held by A's queued rwork keeps A in conn_idr. On every later pass
idr_get_next() returns A again, so the loop never reaches B.

tipc_conn_recv_work() keeps running while B is connected and requests
keep arriving:

    while (connected(con)) {
        if (tipc_conn_rcv_from_sock(con))
            break;
    ...

The cond_resched() here gives up the CPU. A's work still cannot run on
the ordered queue until B's worker returns. If B's receive queue never
empties, idr_in_use never reaches 0 and cleanup_net() spins.

The old loop advanced with id++. It would have cleared CF_CONNECTED on B,
which ends B's worker.

Local peers probably cannot keep this going. The listener is bound with
TIPC_NODE_SCOPE, and local user sockets hold a reference on the netns.

Remote peers are another matter. tipc_net_ops is registered before
tipc_topsrv_net_ops, so the bearers are still up when tipc_topsrv_stop()
runs. The TIPC_LISTEN branch of tipc_sk_filter_connect() has no scope
check. A remote node that knows the listener's port id might be able to
connect and keep sending TIPC_SUB_CANCEL requests. That precondition has
not been confirmed.

Would it work to first walk forward through all entries, using
idr_get_next() from id + 1 and closing each one? The loop could then
retry from 0 only for the entries still waiting on asynchronous release.

> +		if (con && !kref_get_unless_zero(&con->kref))
> +			con = NULL;
> +		spin_unlock_bh(&srv->idr_lock);
>  		if (con) {
> -			conn_get(con);
> -			spin_unlock_bh(&srv->idr_lock);
>  			tipc_conn_close(con);

[Severity: High]

This is a pre-existing issue, but can this call to tipc_conn_close()
dereference a NULL con->sock?

tipc_topsrv_kern_subscr() creates conn_idr entries that have no socket:

    con = tipc_conn_alloc(tipc_topsrv(net), NULL);

tipc_conn_close() dereferences the socket before it tests CF_CONNECTED:

    static void tipc_conn_close(struct tipc_conn *con)
    {
        struct sock *sk = con->sock->sk;

Other code in this file already allows for a NULL sock.
tipc_conn_kref_release() checks if (con->sock). tipc_conn_send_to_sock()
only calls tipc_conn_close() inside its if (con->sock) branch.

Here is a possible sequence:

1. tipc_topsrv_queue_evt() takes a reference on a kernel subscription
   connection K through tipc_conn_lookup(). It then queues K->swork on
   send_wq.

2. Before that work runs, the group socket is closed:
   tipc_release()->tipc_group_delete()->tipc_topsrv_kern_unsubscr().
   This clears CF_CONNECTED and drops two references. K stays in
   conn_idr, held only by the swork reference.

3. cleanup_net()->tipc_topsrv_exit_net()->tipc_topsrv_stop() runs.
   cancel_work_sync(&srv->awork) does not drain send_wq. idr_get_next()
   returns K, kref_get_unless_zero() succeeds, and tipc_conn_close(K)
   dereferences NULL.

The old idr_find() plus conn_get() loop had the same exposure. The new
loop retries until idr_in_use reaches 0, so any socketless entry still in
conn_idr is now guaranteed to reach tipc_conn_close() at least once.

This loop is being rewritten anyway. Could it skip tipc_conn_close() when
con->sock is NULL? Alternatively, tipc_conn_close() could check
con->sock before using it.

>  			conn_put(con);
> -			spin_lock_bh(&srv->idr_lock);
>  		}
> +		cond_resched();
> +		spin_lock_bh(&srv->idr_lock);
>  	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261003192530.2414682-2-Jeremy.Jean%40oss.cyber.gouv.fr

Re: [PATCH] tipc: fix use-after-free in netns teardown

[Jérémy Jean]

On 2026-10-04 21:27, netdev-bot+sashiko@kernel.org wrote:
> [Severity: Medium]
> 
> Can restarting from id 0 on every pass livelock the teardown?
> 
> rcv_wq is an ordered workqueue:
> 
> tipc_topsrv_work_start()
>     s->rcv_wq = alloc_ordered_workqueue("tipc_rcv", 0);
> 
> Take a lower-id connection A whose rwork is queued behind the running
> rwork of a higher-id connection B. The loop closes A, but the reference
> held by A's queued rwork keeps A in conn_idr. On every later pass
> idr_get_next() returns A again, so the loop never reaches B.
> 
> tipc_conn_recv_work() keeps running while B is connected and requests
> keep arriving:
> 
>     while (connected(con)) {
>         if (tipc_conn_rcv_from_sock(con))
>             break;
>     ...
> 
> The cond_resched() here gives up the CPU. A's work still cannot run on
> the ordered queue until B's worker returns. If B's receive queue never
> empties, idr_in_use never reaches 0 and cleanup_net() spins.
> 
> The old loop advanced with id++. It would have cleared CF_CONNECTED on 
> B,
> which ends B's worker.
> 
> Local peers probably cannot keep this going. The listener is bound with
> TIPC_NODE_SCOPE, and local user sockets hold a reference on the netns.
> 
> Remote peers are another matter. tipc_net_ops is registered before
> tipc_topsrv_net_ops, so the bearers are still up when 
> tipc_topsrv_stop()
> runs. The TIPC_LISTEN branch of tipc_sk_filter_connect() has no scope
> check. A remote node that knows the listener's port id might be able to
> connect and keep sending TIPC_SUB_CANCEL requests. That precondition 
> has
> not been confirmed.
> 
> Would it work to first walk forward through all entries, using
> idr_get_next() from id + 1 and closing each one? The loop could then
> retry from 0 only for the entries still waiting on asynchronous 
> release.

This looks like a valid concern. I will address it in a v2.

> 
>> +		if (con && !kref_get_unless_zero(&con->kref))
>> +			con = NULL;
>> +		spin_unlock_bh(&srv->idr_lock);
>>  		if (con) {
>> -			conn_get(con);
>> -			spin_unlock_bh(&srv->idr_lock);
>>  			tipc_conn_close(con);
> 
> [Severity: High]
> 
> This is a pre-existing issue, but can this call to tipc_conn_close()
> dereference a NULL con->sock?

Indeed, Sashiko is right. I reproduced it.

I will fix it in a seperate commit in the v2, by guarding 
tipc_conn_close()
behind a check on con->sock:

diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
index af530c9..01dd34b 100644
--- a/net/tipc/topsrv.c
+++ b/net/tipc/topsrv.c
@@ -706,7 +706,8 @@ static void tipc_topsrv_stop(struct net *net)
                 if (con) {
                         conn_get(con);
                         spin_unlock_bh(&srv->idr_lock);
-                       tipc_conn_close(con);
+                       if (con->sock)
+                               tipc_conn_close(con);
                         conn_put(con);
                         spin_lock_bh(&srv->idr_lock);
                 }


> tipc_topsrv_kern_subscr() creates conn_idr entries that have no socket:
> 
>     con = tipc_conn_alloc(tipc_topsrv(net), NULL);
> 
> tipc_conn_close() dereferences the socket before it tests CF_CONNECTED:
> 
>     static void tipc_conn_close(struct tipc_conn *con)
>     {
>         struct sock *sk = con->sock->sk;
> 
> Other code in this file already allows for a NULL sock.
> tipc_conn_kref_release() checks if (con->sock). 
> tipc_conn_send_to_sock()
> only calls tipc_conn_close() inside its if (con->sock) branch.
> 
> Here is a possible sequence:
> 
> 1. tipc_topsrv_queue_evt() takes a reference on a kernel subscription
>    connection K through tipc_conn_lookup(). It then queues K->swork on
>    send_wq.
> 
> 2. Before that work runs, the group socket is closed:
>    tipc_release()->tipc_group_delete()->tipc_topsrv_kern_unsubscr().
>    This clears CF_CONNECTED and drops two references. K stays in
>    conn_idr, held only by the swork reference.
> 
> 3. cleanup_net()->tipc_topsrv_exit_net()->tipc_topsrv_stop() runs.
>    cancel_work_sync(&srv->awork) does not drain send_wq. idr_get_next()
>    returns K, kref_get_unless_zero() succeeds, and tipc_conn_close(K)
>    dereferences NULL.
> 
> The old idr_find() plus conn_get() loop had the same exposure. The new
> loop retries until idr_in_use reaches 0, so any socketless entry still 
> in
> conn_idr is now guaranteed to reach tipc_conn_close() at least once.
> 
> This loop is being rewritten anyway. Could it skip tipc_conn_close() 
> when
> con->sock is NULL? Alternatively, tipc_conn_close() could check
> con->sock before using it.
> 
>>  			conn_put(con);
>> -			spin_lock_bh(&srv->idr_lock);
>>  		}
>> +		cond_resched();
>> +		spin_lock_bh(&srv->idr_lock);
>>  	}
> 
> [ ... ]