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@xxxxxxxxxxx>
>
>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@xxxxxxxxxx
>Reported-by: TencentOS Corvus AI <corvus@xxxxxxxxxxx>
>Signed-off-by: Jun Yang <junvyyang@xxxxxxxxxxx>
>---
> 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
>