[PATCH net] tipc: purge cong_links under the socket lock in tipc_release()

[PATCH net] tipc: purge cong_links under the socket lock in tipc_release()

[Jun Yang]

From: Jun Yang <junvyyang@tencent.com>

tipc_release() frees the elements of tsk->cong_links after release_sock(),
i.e. with no lock held:

	tipc_sk_remove(tsk);
	sock_orphan(sk);
	release_sock(sk);
	tipc_dest_list_purge(&tsk->cong_links);	/* no lock */
	tsk->cong_link_cnt = 0;

Every other accessor of that list runs under the socket lock, including
the SOCK_WAKEUP handler in tipc_sk_proto_rcv(), which does
tipc_dest_del(&tsk->cong_links, ...) while holding only sk->sk_lock.slock
via tipc_sk_rcv()'s spin_trylock_bh().  Because the purge never acquires
that spinlock, it provides no mutual exclusion against the wakeup path.

A SOCK_WAKEUP delivered for this port can therefore run concurrently with
the purge: tipc_sk_rcv() looks the socket up and takes a reference before
tipc_sk_remove() unhashes it, is then delayed past release_sock() so
sock_owned_by_user() is false, its spin_trylock_bh() succeeds, and it
list_del()s and kfree()s a struct tipc_dest that the closing task is
walking at the same time.  Both paths free entries of the same list.

__tipc_shutdown() does not close this window: it waits on
!tsk->cong_link_cnt but ignores the return value of tipc_wait_for_cond(),
which returns early on timeout, on a pending signal, or on sk_err, so the
close can proceed with cong_links still populated.

  BUG: KASAN: slab-use-after-free in __list_del_entry_valid_or_report+0x1ce/0x280
  Read of size 8 at addr ffff888105ddda88 by task poc_cong_race/7856
   __list_del_entry_valid_or_report+0x1ce/0x280
   tipc_dest_list_purge+0xad/0x240
   tipc_release+0x9a5/0x1340
  Allocated by task 7856:
   tipc_dest_push+0x11c/0x2f0
   __tipc_sendmsg+0x1443/0x17a0
  Freed by task 7851:
   tipc_dest_del+0x1ce/0x280
   tipc_sk_filter_rcv+0x1da7/0x2e70
   tipc_sk_rcv+0xdaa/0x1a80
   tipc_udp_recv+0x42a/0x7e0
  Oops: general protection fault ... Kernel panic - not syncing: Fatal exception

Move the purge above release_sock() so it runs under the socket lock, the
same discipline commit 844cf763fba6 ("tipc: make macro tipc_wait_for_cond()
smp safe") established for the wait condition.  A concurrent tipc_sk_rcv()
then either backlogs the wakeup because the socket is owned, or processes
it against an already empty list, and no new delivery can arrive because
tipc_sk_remove() has already unhashed the socket.

Fixes: 365ad353c256 ("tipc: reduce risk of user starvation during link congestion")
Cc: stable@kernel.org
Reported-by: TencentOS Corvus AI <corvus@tencent.com>
Signed-off-by: Jun Yang <junvyyang@tencent.com>
---
 net/tipc/socket.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index d5d70eb230b5..9b45b16d0a31 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -647,11 +647,17 @@ static int tipc_release(struct socket *sock)
 	sk_stop_timer(sk, &sk->sk_timer);
 	tipc_sk_remove(tsk);
 
+	/* Purge under the socket lock: a straggler SOCK_WAKEUP that looked the
+	 * socket up before tipc_sk_remove() can still reach tipc_dest_del() on
+	 * this list, and it only holds the socket spinlock.  Purging after
+	 * release_sock() would race that list_del()/kfree().
+	 */
+	tipc_dest_list_purge(&tsk->cong_links);
+	tsk->cong_link_cnt = 0;
+
 	sock_orphan(sk);
 	/* Reject any messages that accumulated in backlog queue */
 	release_sock(sk);
-	tipc_dest_list_purge(&tsk->cong_links);
-	tsk->cong_link_cnt = 0;
 	call_rcu(&tsk->rcu, tipc_sk_callback);
 	sock->sk = NULL;
 
-- 
2.55.0

RE: [PATCH net] tipc: purge cong_links under the socket lock in tipc_release()

[Tung Quang Nguyen]

>Subject: [PATCH net] tipc: purge cong_links under the socket lock in
>tipc_release()
>
>From: Jun Yang <junvyyang@tencent.com>
>
>tipc_release() frees the elements of tsk->cong_links after release_sock(), i.e.
>with no lock held:
>
>	tipc_sk_remove(tsk);
>	sock_orphan(sk);
>	release_sock(sk);
>	tipc_dest_list_purge(&tsk->cong_links);	/* no lock */
>	tsk->cong_link_cnt = 0;
>
>Every other accessor of that list runs under the socket lock, including the
>SOCK_WAKEUP handler in tipc_sk_proto_rcv(), which does tipc_dest_del(&tsk-
>>cong_links, ...) while holding only sk->sk_lock.slock via tipc_sk_rcv()'s
>spin_trylock_bh().  Because the purge never acquires that spinlock, it provides
>no mutual exclusion against the wakeup path.
>
>A SOCK_WAKEUP delivered for this port can therefore run concurrently with
>the purge: tipc_sk_rcv() looks the socket up and takes a reference before
>tipc_sk_remove() unhashes it, is then delayed past release_sock() so
>sock_owned_by_user() is false, its spin_trylock_bh() succeeds, and it list_del()s
>and kfree()s a struct tipc_dest that the closing task is walking at the same time.
>Both paths free entries of the same list.
>
>__tipc_shutdown() does not close this window: it waits on !tsk->cong_link_cnt
>but ignores the return value of tipc_wait_for_cond(), which returns early on
>timeout, on a pending signal, or on sk_err, so the close can proceed with
>cong_links still populated.
>
>  BUG: KASAN: slab-use-after-free in
>__list_del_entry_valid_or_report+0x1ce/0x280
>  Read of size 8 at addr ffff888105ddda88 by task poc_cong_race/7856
>   __list_del_entry_valid_or_report+0x1ce/0x280
>   tipc_dest_list_purge+0xad/0x240
>   tipc_release+0x9a5/0x1340
>  Allocated by task 7856:
>   tipc_dest_push+0x11c/0x2f0
>   __tipc_sendmsg+0x1443/0x17a0
>  Freed by task 7851:
>   tipc_dest_del+0x1ce/0x280
>   tipc_sk_filter_rcv+0x1da7/0x2e70
>   tipc_sk_rcv+0xdaa/0x1a80
>   tipc_udp_recv+0x42a/0x7e0

Please update the commit message with decoded stack trace (using linux/scripts/decode_stacktrace.sh).

>  Oops: general protection fault ... Kernel panic - not syncing: Fatal exception
>
>Move the purge above release_sock() so it runs under the socket lock, the
>same discipline commit 844cf763fba6 ("tipc: make macro tipc_wait_for_cond()
>smp safe") established for the wait condition.  A concurrent tipc_sk_rcv() then
>either backlogs the wakeup because the socket is owned, or processes it
>against an already empty list, and no new delivery can arrive because
>tipc_sk_remove() has already unhashed the socket.
>
>Fixes: 365ad353c256 ("tipc: reduce risk of user starvation during link
>congestion")
>Cc: stable@kernel.org
>Reported-by: TencentOS Corvus AI <corvus@tencent.com>
>Signed-off-by: Jun Yang <junvyyang@tencent.com>
>---
> net/tipc/socket.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
>diff --git a/net/tipc/socket.c b/net/tipc/socket.c index
>d5d70eb230b5..9b45b16d0a31 100644
>--- a/net/tipc/socket.c
>+++ b/net/tipc/socket.c
>@@ -647,11 +647,17 @@ static int tipc_release(struct socket *sock)
> 	sk_stop_timer(sk, &sk->sk_timer);
> 	tipc_sk_remove(tsk);
>
>+	/* Purge under the socket lock: a straggler SOCK_WAKEUP that looked
>the
>+	 * socket up before tipc_sk_remove() can still reach tipc_dest_del() on
>+	 * this list, and it only holds the socket spinlock.  Purging after
>+	 * release_sock() would race that list_del()/kfree().
>+	 */
>+	tipc_dest_list_purge(&tsk->cong_links);
>+	tsk->cong_link_cnt = 0;
>+
> 	sock_orphan(sk);
> 	/* Reject any messages that accumulated in backlog queue */
> 	release_sock(sk);
>-	tipc_dest_list_purge(&tsk->cong_links);
>-	tsk->cong_link_cnt = 0;
> 	call_rcu(&tsk->rcu, tipc_sk_callback);
> 	sock->sk = NULL;
>
>--
>2.55.0
>