[PATCH net] net/smc: protect clcsock lifetime in smc_getname

[PATCH net] net/smc: protect clcsock lifetime in smc_getname

[Chengfeng Ye]

smc_getname() dereferences smc->clcsock without holding
clcsock_release_lock. Link-group termination can release the CLC socket
through smc_close_active_abort() while the SMC socket is still open,
for example after shutdown(SHUT_WR).

A getsockname() caller can load smc->clcsock, then the termination worker
can clear the pointer and call sock_release() before the caller accesses
clcsock->ops or invokes getname(). This causes a use-after-free; if the
worker clears the pointer before the load, it causes a NULL dereference.
The syscall's file reference keeps the SMC socket alive but does not
prevent asynchronous release of its CLC socket.

KASAN reported:

  BUG: KASAN: slab-use-after-free in smc_getname+0x19e/0x1b0
  Read of size 8 at addr ffff888109abb4e0 by task poc/103
  Call Trace:
   smc_getname+0x19e/0x1b0
   do_getsockname+0xe5/0x170
   __sys_getsockname+0x8c/0x100

  Allocated by task 95:
   sock_alloc_inode+0x1e/0x280
   sock_alloc+0x3d/0x240
   __sock_create+0x7e/0x430
   smc_create+0x121/0x240

  Freed by task 0:
   kmem_cache_free+0xcc/0x340
   rcu_core+0x50a/0x1850

  Last potentially related work creation:
   evict+0x446/0x6c0
   smc_clcsock_release+0xa8/0xd0
   smc_close_active_abort+0x26a/0x3a0
   __smc_lgr_terminate.part.0+0x137/0x2e0

Hold clcsock_release_lock across the pointer check and the getname()
callback, as the socket option handlers already do. Return -EBADF if
the CLC socket has already been released, preserving the existing peer
state check and the callback's return value otherwise.

Fixes: b03faa1fafc8 ("net/smc: postpone release of clcsock")
Cc: stable@vger.kernel.org
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
---
 net/smc/af_smc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index e9f93b3ab435..8b3ee70f8433 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -2784,6 +2784,7 @@ int smc_getname(struct socket *sock, struct sockaddr *addr,
 		int peer)
 {
 	struct smc_sock *smc;
+	int rc = -EBADF;
 
 	if (peer && (sock->sk->sk_state != SMC_ACTIVE) &&
 	    (sock->sk->sk_state != SMC_APPCLOSEWAIT1))
@@ -2791,7 +2792,11 @@ int smc_getname(struct socket *sock, struct sockaddr *addr,
 
 	smc = smc_sk(sock->sk);
 
-	return smc->clcsock->ops->getname(smc->clcsock, addr, peer);
+	mutex_lock(&smc->clcsock_release_lock);
+	if (smc->clcsock)
+		rc = smc->clcsock->ops->getname(smc->clcsock, addr, peer);
+	mutex_unlock(&smc->clcsock_release_lock);
+	return rc;
 }
 
 int smc_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
-- 
2.43.0

Re: [PATCH net] net/smc: protect clcsock lifetime in smc_getname

[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] The patch fixes smc_getname(), and its commit message says
  socket option handlers already guard clcsock.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  smc_diag_msg_common_fill() (net/smc/smc_diag.c:42-55) has the same…
- [High] This problem wasn't introduced by this patch, but smc_shutdown()
  (net/smc/af_smc.c:3011-3012) runs `if (do_shutdown && smc->clcsock)…

--

net/smc: protect clcsock lifetime in smc_getname

This commit takes clcsock_release_lock in smc_getname() around the
smc->clcsock NULL check and the getname() callback. If link-group
termination has already released the CLC socket through
smc_close_active_abort(), it returns -EBADF.

> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index e9f93b3ab435b..8b3ee70f8433f 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c

[ ... ]

> @@ -2791,7 +2792,11 @@ int smc_getname(struct socket *sock, struct sockaddr *addr,
>  
>  	smc = smc_sk(sock->sk);
>  
> -	return smc->clcsock->ops->getname(smc->clcsock, addr, peer);
> +	mutex_lock(&smc->clcsock_release_lock);
> +	if (smc->clcsock)
> +		rc = smc->clcsock->ops->getname(smc->clcsock, addr, peer);
> +	mutex_unlock(&smc->clcsock_release_lock);
> +	return rc;
>  }

[Severity: High]
The commit message says:

    Hold clcsock_release_lock across the pointer check and the getname()
    callback, as the socket option handlers already do.

Does this hold for every socket option path? SMC does not set
SOCK_CUSTOM_SOCKOPT. So setsockopt(fd, SOL_SOCKET, SO_KEEPALIVE, ...) on
an AF_SMC socket never reaches smc_setsockopt():

do_sock_setsockopt()
  sock_setsockopt()
    sk_setsockopt()
      sk->sk_prot->keepalive(sk, valbool)
        smc_set_keepalive()

smc_set_keepalive() in net/smc/af_smc.c uses clcsock without a NULL check
and without clcsock_release_lock:

static void smc_set_keepalive(struct sock *sk, int val)
{
	struct smc_sock *smc = smc_sk(sk);

	smc->clcsock->sk->sk_prot->keepalive(smc->clcsock->sk, val);
}

Take the sequence from the commit message: shutdown(SHUT_WR), then
link-group termination. smc_close_active_abort() calls
smc_clcsock_release(). That sets smc->clcsock to NULL and calls
sock_release() while the SMC file is still open.

Would a later setsockopt(SO_KEEPALIVE) then hit a NULL pointer dereference
in smc_set_keepalive()? And if it runs while smc_clcsock_release() is in
progress, could it become a use-after-free of the CLC socket?

This is a pre-existing issue and was not introduced by this patch.
smc_diag_msg_common_fill() in net/smc/smc_diag.c reads clcsock without
a lock, the same pattern this patch removes from smc_getname():

	if (!smc->clcsock)
		return;
	r->id.idiag_sport = htons(smc->clcsock->sk->sk_num);
	r->id.idiag_dport = smc->clcsock->sk->sk_dport;
	r->id.idiag_if = smc->clcsock->sk->sk_bound_dev_if;
	...

[Severity: High]
The dump path is
smc_diag_dump_proto()->__smc_diag_dump()->smc_diag_msg_common_fill(). It
holds only read_lock(&prot->h.smc_hash->lock).

The comment in smc_inet_destroy_sock() says the diag dumps stay safe only
if the socket is unhashed before smc_clcsock_release(). __smc_release()
does this. smc_close_active_abort() does not: it releases clcsock while
the SMC socket is still hashed. It holds only clcsock_release_lock, which
the diag reader never takes.

Each store to r->id can force a reload of smc->clcsock. Can a dump pass
the NULL check and then reload smc->clcsock as NULL? Could it also load
clcsock->sk after __sock_release() has set it to NULL? The AF_SMC diag
dump needs no privileges, so the result would be a NULL dereference
inside the read_lock section.

A mutex can't be taken under the rwlock. Would one of these work here?

  - A single READ_ONCE() snapshot of clcsock and clcsock->sk, with NULL
    checks, relying on the RCU-deferred socket free.
  - Unhashing the socket before the release on the abort path.

[Severity: High]
This is also a pre-existing issue and was not introduced by this patch.
smc_shutdown() in net/smc/af_smc.c uses clcsock while holding only
lock_sock(sk):

	if (do_shutdown && smc->clcsock)
		rc1 = kernel_sock_shutdown(smc->clcsock, how);

smc_close_active_abort() drops lock_sock before it releases clcsock:

	if (release_clcsock) {
		release_sock(sk);
		smc_clcsock_release(smc);
		lock_sock(sk);
	}

This means lock_sock(sk) alone does not keep clcsock alive. One possible
interleaving:

Thread A: shutdown(SHUT_WR) while in SMC_ACTIVE
  smc_close_shutdown_write()
    release_sock(sk);
    cancel_delayed_work_sync(&conn->tx_work);

Thread B: shutdown(SHUT_RDWR) or shutdown(SHUT_WR) on the same fd
  socket moves to SMC_PEERCLOSEWAIT1

Termination worker:
  __smc_lgr_terminate()->smc_conn_kill()->smc_close_active_abort()
    sk_state = SMC_CLOSED, release_clcsock = true
    release_sock(sk);

Thread A:
    lock_sock(sk);
    state != SMC_ACTIVE, goto again, no case matches, returns
  smc_shutdown()
    smc->clcsock is still non-NULL
    kernel_sock_shutdown(smc->clcsock, how);

Termination worker, concurrently:
    smc_clcsock_release(smc)
      __sock_release()
        sock->sk = NULL;
        sock->ops = NULL;
        iput(SOCK_INODE(sock));

Could this be a NULL dereference or a use-after-free of the CLC socket?
Thread A can sleep in lock_sock() inside inet_shutdown(), outside any RCU
read-side section.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260926180404.2721010-1-nicoyip.dev%40gmail.com