[PATCH] net/smc: reject CDC messages after connection teardown

[PATCH] net/smc: reject CDC messages after connection teardown

[Weiming Shi]

smc_cdc_rx_handler() drops the link group's connection lock after looking
up a connection. Concurrent close(2) can then set conn->freed and release a
registration-failed receive buffer before the receive handler obtains the
socket lock. The handler subsequently dereferences the released descriptor.

KASAN reports:

  BUG: KASAN: slab-use-after-free in smc_cdc_msg_recv (net/smc/smc_cdc.c:376 net/smc/smc_cdc.c:431)
  Read of size 4 at addr ffff8881064a4720 by task kworker/u48:2/233
  Workqueue: rxe_wq do_work
  Call Trace:
   <IRQ>
   smc_cdc_msg_recv (net/smc/smc_cdc.c:376 net/smc/smc_cdc.c:431)
   smc_cdc_rx_handler (net/smc/smc_cdc.c:503)
   smc_wr_rx_tasklet_fn (net/smc/smc_wr.c:445 net/smc/smc_wr.c:459 net/smc/smc_wr.c:498)
   tasklet_action_common (kernel/softirq.c:961)
   handle_softirqs (kernel/softirq.c:645)
   do_softirq (kernel/softirq.c:546 (discriminator 19) kernel/softirq.c:533 (discriminator 19))
   </IRQ>

  Freed by task 209:
   __kasan_slab_free (mm/kasan/common.c:285)
   kfree (./include/linux/kasan.h:235 mm/slub.c:2748
          mm/slub.c:6508 mm/slub.c:6801)
   smc_conn_free (net/smc/smc_core.c:1244 net/smc/smc_core.c:1283)
   __smc_release (net/smc/af_smc.c:322)
   smc_release (net/smc/af_smc.c:361)
   __sock_release (net/socket.c:735)
   sock_close (net/socket.c:1526 (discriminator 1))
   __fput (fs/file_table.c:512)
   fput_close_sync (fs/file_table.c:617)
   __do_sys_close (fs/open.c:1560)

  The buggy address is located 32 bytes inside of
   freed 128-byte region [ffff8881064a4700, ffff8881064a4780)
  Kernel panic - not syncing: KASAN: panic_on_warn set ...

Both smc_conn_free() and CDC message processing run under the socket lock.
Check conn->freed after acquiring that lock. Discard messages which passed
the lookup before teardown. This keeps the existing lock order and prevents
access to receive buffers released by smc_conn_free().

Fixes: a6920d1d130c ("net/smc: handle unregistered buffers")
Cc: stable@vger.kernel.org
Reported-by: Xiang Mei <xmei5@asu.edu>
Assisted-by: LLM
Signed-off-by: Weiming Shi <bestswngs@gmail.com>
---
 net/smc/smc_cdc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
index 32d6d03df321..499a2473b79b 100644
--- a/net/smc/smc_cdc.c
+++ b/net/smc/smc_cdc.c
@@ -428,7 +428,8 @@ static void smc_cdc_msg_recv(struct smc_sock *smc, struct smc_cdc_msg *cdc)
 {
 	sock_hold(&smc->sk);
 	bh_lock_sock(&smc->sk);
-	smc_cdc_msg_recv_action(smc, cdc);
+	if (!smc->conn.freed)
+		smc_cdc_msg_recv_action(smc, cdc);
 	bh_unlock_sock(&smc->sk);
 	sock_put(&smc->sk); /* no free sk in softirq-context */
 }
-- 
2.56.0

Re: [PATCH] net/smc: reject CDC messages after connection teardown

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] net/smc: bh_lock_sock() in smc_cdc_msg_recv() does not exclude smc_conn_free()
--

commit 64de84c513a98ece4d44144df98874a465343ddf
Author: Weiming Shi <bestswngs@gmail.com>

net/smc: reject CDC messages after connection teardown

This commit checks smc->conn.freed in smc_cdc_msg_recv() after acquiring
bh_lock_sock(&smc->sk) to avoid processing CDC messages and dereferencing
released receive buffer descriptors after connection teardown.

> diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
> index 32d6d03df3214..499a2473b79b4 100644
> --- a/net/smc/smc_cdc.c
> +++ b/net/smc/smc_cdc.c
> @@ -428,7 +428,8 @@ static void smc_cdc_msg_recv(struct smc_sock *smc, struct smc_cdc_msg *cdc)
>  {
>  	sock_hold(&smc->sk);
>  	bh_lock_sock(&smc->sk);
> -	smc_cdc_msg_recv_action(smc, cdc);
> +	if (!smc->conn.freed)
> +		smc_cdc_msg_recv_action(smc, cdc);
>  	bh_unlock_sock(&smc->sk);
>  	sock_put(&smc->sk); /* no free sk in softirq-context */
>  }

[Severity: Critical]
Does checking smc->conn.freed under bh_lock_sock(&smc->sk) actually serialize
smc_cdc_msg_recv() against smc_conn_free()?

Process-context callers of smc_conn_free(), such as smc_release() ->
__smc_release() -> smc_conn_free(), hold the sleeping socket lock via
lock_sock(&smc->sk). In net/core/sock.c:lock_sock_nested(), acquiring the
user-context socket lock sets sk->sk_lock.owned = 1 and immediately drops
sk->sk_lock.slock so the caller can sleep:

net/core/sock.c:lock_sock_nested() {
	spin_lock_bh(&sk->sk_lock.slock);
	if (unlikely(sock_owned_by_user_nocheck(sk)))
		__lock_sock(sk);
	sk->sk_lock.owned = 1;
	spin_unlock_bh(&sk->sk_lock.slock);
}

Meanwhile, smc_cdc_msg_recv() runs in tasklet context via
smc_wr_rx_tasklet_fn() -> smc_cdc_rx_handler() -> smc_cdc_msg_recv() and
calls bh_lock_sock(&smc->sk), which only acquires sk->sk_lock.slock without
waiting on sk->sk_lock.owned. In fact, smc_cdc_msg_recv_action() in
net/smc/smc_cdc.c explicitly checks sock_owned_by_user(&smc->sk) because it
can run while a process holds lock_sock(&smc->sk):

net/smc/smc_cdc.c:smc_cdc_msg_recv_action() {
	...
		if (!sock_owned_by_user(&smc->sk))
			smc_tx_pending(conn);
		else
			conn->tx_in_release_sock = true;
	...
}

In net/smc/smc_core.c:smc_conn_free(), conn->freed = 1 and smc_buf_unuse()
run without holding bh_lock_sock(&smc->sk) and before
smc_lgr_unregister_conn(conn):

net/smc/smc_core.c:smc_conn_free() {
	...
	conn->freed = 1;
	...
	if (!list_empty(&lgr->list)) {
		smc_buf_unuse(conn, lgr); /* allow buffer reuse */
		smc_lgr_unregister_conn(conn);
	}
	...
}

If CPU 0 enters smc_cdc_msg_recv(), acquires bh_lock_sock(&smc->sk), and sees
!smc->conn.freed just before CPU 1 in smc_conn_free() sets conn->freed = 1
and frees conn->rmb_desc or conn->sndbuf_desc via smc_buf_unuse(), can CPU 0
still concurrently dereference the freed conn->rmb_desc->len in
smc_cdc_msg_recv_action():

net/smc/smc_cdc.c:smc_cdc_msg_recv_action() {
	...
	diff_prod = smc_curs_diff(conn->rmb_desc->len, &prod_old,
				  &conn->local_rx_ctrl.prod);
	...
}

Additionally, if smc_cdc_msg_recv_action() observes
sock_owned_by_user(&smc->sk) while __smc_release() holds lock_sock(&smc->sk),
won't setting conn->tx_in_release_sock = true cause the subsequent
release_sock(&smc->sk) in smc_release() to invoke smc_release_cb() ->
smc_tx_pending() on a connection that smc_conn_free() has already torn down?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021171256.166870-2-bestswngs@gmail.com?part=1