[PATCH v2 0/2] tipc: fix topology server teardown

[PATCH v2 0/2] tipc: fix topology server teardown

[Jérémy Jean]

This series fixes a connection UAF and a NULL-socket dereference during
topology-server teardown.

Changes since v1, addressing Sashiko's review:
- Add guard to con->sock to fix NULL dereference.
- Walk forward through conn_idr before draining the workqueues.

v1: https://lore.kernel.org/all/20261003192530.2414682-2-Jeremy.Jean@oss.cyber.gouv.fr/

Jérémy Jean (2):
  tipc: skip socketless connections in topsrv teardown
  tipc: fix use-after-free in netns teardown

 net/tipc/topsrv.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

-- 
2.47.3

[PATCH v2 1/2] tipc: skip socketless connections in topsrv teardown

[Jérémy Jean]

Kernel subscription connections have no socket but can remain in conn_idr
after unsubscribe while send work is pending. 

Skip tipc_conn_close() for these entries to avoid a NULL dereference
during netns teardown.

Fixes: e88f2be83282 ("tipc: fix race condition at topology server receive")
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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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);
 		}
-- 
2.47.3

RE: [PATCH v2 1/2] tipc: skip socketless connections in topsrv teardown

[Tung Quang Nguyen]

>Subject: [PATCH v2 1/2] tipc: skip socketless connections in topsrv teardown
>
>Kernel subscription connections have no socket but can remain in conn_idr
>after unsubscribe while send work is pending.
>
>Skip tipc_conn_close() for these entries to avoid a NULL dereference during
>netns teardown.
>

This does not fix the root cause.
I fixed the issue in this patch: https://git.kernel.org/netdev/net/c/3acdd44385bc
Please update your net tree to check out  the fix.

--
pw-bot: rejected

Re: [PATCH v2 1/2] tipc: skip socketless connections in topsrv teardown

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (2) · Medium: 0 · 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).

- [High] This patch is not complete on its own.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the same
  livelock already exists for socket-backed conns.
- [High] This problem wasn't introduced by this patch, but
  tipc_topsrv_stop() calls conn_get() (plain kref_get()) on any conn…

--

tipc: skip socketless connections in topsrv teardown

This patch makes tipc_topsrv_stop() skip tipc_conn_close() for kernel
subscription connections, which have no socket. These connections can
stay in conn_idr after unsubscribe while send work is still pending.
Calling tipc_conn_close() on them dereferences a NULL con->sock during
netns teardown.

> diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
> index af530c9ed8402..01dd34b758f4b 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);
>  		}

[Severity: High]
This avoids the NULL dereference. In the scenario from the commit
message, though, can netns teardown still stall and then hit a
use-after-free on con?

In that scenario, tipc_topsrv_queue_evt() passes its lookup reference
to con->swork:

	if (queue_work(srv->send_wq, &con->swork))
		return;

tipc_topsrv_kern_unsubscr() then drops its two references:

	test_and_clear_bit(CF_CONNECTED, &con->flags);
	tipc_conn_delete_sub(con, NULL);
	conn_put(con);
	conn_put(con);

After that, the pending swork holds the last reference.

At this commit the loop in tipc_topsrv_stop() is:

	spin_lock_bh(&srv->idr_lock);
	for (id = 0; srv->idr_in_use; id++) {
		con = idr_find(&srv->conn_idr, id);
		if (con) {
		[ ... ]

With the new check, the socketless conn gets conn_get() and conn_put(),
which leaves the refcount at 1. The loop then retakes idr_lock and moves
on.

idr_find() returns NULL for every higher id. So idr_lock stays held with
BH disabled and no cond_resched().

Meanwhile the send_wq worker runs:

tipc_conn_send_work()
    conn_put(con)
        tipc_conn_kref_release()
            spin_lock_bh(&s->idr_lock);   <- blocks on the stop loop

tipc_conn_kref_release() is the only place that calls idr_remove() and
decrements s->idr_in_use. So the loop only ends after int id wraps
around, which takes about 2^32 lookups under the spinlock. Won't that
cause soft lockup and RCU stall warnings?

When id gets back to the conn's slot, conn_get() raises a zero refcount.
After spin_unlock_bh(), the release path calls kfree(con). The stop path
then reads con->sock and calls conn_put() on freed memory.

The path is reachable from an unprivileged user in a user+net namespace:

  1. TIPC_GROUP_JOIN (tipc_group_create()->tipc_topsrv_kern_subscr()).
  2. Leave the group or close the socket while topology events are
     queued (tipc_group_delete()->tipc_topsrv_kern_unsubscr()).
  3. Tear down the netns.

The later patch in this series, "tipc: fix use-after-free in netns
teardown", fixes this. Its loop uses idr_get_next_ul() with
kref_get_unless_zero() and drops idr_lock with cond_resched() on every
iteration. Remaining work references are left to
tipc_topsrv_work_stop(). So the problem only exists at this commit.

The two patches carry different Fixes: tags. Could that cause trouble
for bisection, or for stable trees that take this patch without the
follow-up?

[Severity: High]
This isn't a bug introduced by this patch, but the same livelock looks
possible for socket-backed conns too.

Suppose recv or send work already cleared CF_CONNECTED through
tipc_conn_close(), while rwork or swork still holds a reference. The
call to tipc_conn_close() from the stop loop then returns early without
a conn_put():

	disconnect = test_and_clear_bit(CF_CONNECTED, &con->flags);
	[ ... ]
	if (!disconnect)
		return;

The loop's own conn_get()/conn_put() pair leaves the work's reference
in place. The loop then scans empty ids under idr_lock, waiting for
idr_in_use to reach 0.

The worker's final conn_put()->tipc_conn_kref_release() blocks in
spin_lock_bh(&s->idr_lock). Can idr_in_use ever drop while the stop
loop spins?

The later patch in this series, "tipc: fix use-after-free in netns
teardown", also fixes this. With it, the loop ends when
idr_get_next_ul() returns NULL and drops idr_lock on every iteration.

[Severity: High]
This is a pre-existing issue, but can the conn_get() here bring back a
zero refcount? conn_get() is a plain kref_get():

static void conn_get(struct tipc_conn *con)
{
	kref_get(&con->kref);
}

tipc_conn_lookup() uses kref_get_unless_zero() instead.

Say a final conn_put() from rwork, swork or tipc_conn_close() runs on
another CPU while the stop loop holds idr_lock. The conn is then still
visible in conn_idr with a refcount of 0:

CPU1                              CPU2 tipc_topsrv_stop()
                                  spin_lock_bh(&srv->idr_lock)
conn_put(con)
  tipc_conn_kref_release()
    spin_lock_bh(&s->idr_lock)
      (blocks)
                                  con = idr_find(&srv->conn_idr, id)
                                  conn_get(con)    <- refcount from 0
                                  spin_unlock_bh(&srv->idr_lock)
    idr_remove(&s->conn_idr, ...)
    [ ... ]
    kfree(con)
                                  if (con->sock)   <- freed memory
                                      tipc_conn_close(con)
                                  conn_put(con)

Before this patch, tipc_conn_close() already dereferenced con->sock->sk
on the same freed object. The new con->sock read touches the same
memory.

This seems to match the KASAN slab-use-after-free described in the next
patch in the series, "tipc: fix use-after-free in netns teardown". That
patch replaces conn_get() with kref_get_unless_zero() under idr_lock
and sets con = NULL when it fails.

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

[PATCH v2 2/2] 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 a 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.
Walk forward through conn_idr to disconnect all connections before
draining the workqueues.

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 | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
index 01dd34b..f3f32fd 100644
--- a/net/tipc/topsrv.c
+++ b/net/tipc/topsrv.c
@@ -698,19 +698,25 @@ static void tipc_topsrv_stop(struct net *net)
 	struct tipc_topsrv *srv = tipc_topsrv(net);
 	struct socket *lsock = srv->listener;
 	struct tipc_conn *con;
-	int id;
+	unsigned long 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; (con = idr_get_next_ul(&srv->conn_idr, &id)); id++) {
+		if (!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);
 			if (con->sock)
 				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 v2 2/2] tipc: fix use-after-free in netns teardown

[Tung Quang Nguyen]

>Subject: [PATCH v2 2/2] tipc: fix use-after-free in netns teardown
>
>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 a 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.
>Walk forward through conn_idr to disconnect all connections before draining
>the workqueues.

This does not fix the root cause.
I fixed the issue in this patch: https://git.kernel.org/netdev/net/c/3acdd44385bc
Please update your net tree to check out  the fix.

--
pw-bot: rejected

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] After this patch, `struct tipc_topsrv::idr_in_use` is dead state.

--

tipc: fix use-after-free in netns teardown

This patch stops the listener and cancels the accept work before
tipc_topsrv_stop() walks conn_idr. The walk now goes forward with
idr_get_next_ul() and uses kref_get_unless_zero(), so it no longer takes
a reference on a connection whose count has already reached zero.
Connections are disconnected before the workqueues are drained.

> diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
> index 01dd34b758f4b..f3f32fd8134ac 100644
> --- a/net/tipc/topsrv.c
> +++ b/net/tipc/topsrv.c

[ ... ]

>  	spin_lock_bh(&srv->idr_lock);
> -	for (id = 0; srv->idr_in_use; id++) {
> -		con = idr_find(&srv->conn_idr, id);
> +	for (id = 0; (con = idr_get_next_ul(&srv->conn_idr, &id)); id++) {

[Severity: Low]
This isn't a bug, but this loop condition was the only place that read
srv->idr_in_use. Is the field still needed after this change?

It is still declared and documented in struct tipc_topsrv:

 * @idr_in_use: amount of allocated identifier entry
 ...
	int idr_in_use;

It is also still written under idr_lock. It is set to 0 in
tipc_topsrv_start(), incremented in tipc_conn_alloc() and decremented in
tipc_conn_kref_release():

	s->idr_in_use++;
	...
	s->idr_in_use--;

Nothing reads it now.

I checked whether dropping the old termination condition removes a
guarantee that all connections are gone before kfree(srv), and it does
not appear to.

tipc_conn_close() unsubscribes synchronously via tipc_conn_delete_sub()
->tipc_sub_unsubscribe() before it drops its reference. That
unsubscribe serializes on sc->lock and calls timer_delete_sync(). The
get/put pairs in tipc_conn_data_ready() and tipc_conn_write_space() are
bounded by sk_callback_lock. The remaining final puts run from
rcv_wq/send_wq, and tipc_topsrv_work_stop() drains those.

The old loop also held idr_lock while stepping over empty ids, so it
could not act as a wait-for-empty barrier anyway.

Could the field and its kernel-doc entry be removed in this patch? If
it is kept, could the commit message say that the old idr_in_use
termination condition was dropped on purpose?

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

[ ... ]

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