Re: [PATCH net v1] net/smc: fix socket use-after-free during link group termination

On 23/07/26 4:24 pm, [email protected] wrote:
> From: Xuanqiang Luo <[email protected]>
> 
> __smc_lgr_terminate() drops conns_lock after finding a connection in
> lgr->conns_all, but before taking a reference on its socket. The connection
> is embedded in the socket, and its registration reference protects it only
> while the connection remains in the tree.
> 
> A concurrent close can unregister the connection and drop that reference,
> freeing the socket before the termination worker reaches sock_hold().
> 
> The race is reachable when close overlaps link group termination.
> Local stress testing reproduced the use-after-free and KASAN reported:
> 
>   BUG: KASAN: slab-use-after-free in __smc_lgr_terminate.part.0 [smc]
>   Write of size 4 by task kworker/3:3
>   Workqueue: events smc_lgr_terminate_work [smc]
>   __smc_lgr_terminate.part.0 [smc]
> 
> The socket was allocated by smc_create(), freed through
> slab_free_after_rcu_debug(), and was followed by:
> 
>   refcount_t: addition on 0; use-after-free.
>   __smc_lgr_terminate.part.0 [smc]
> 
> Take the socket reference while conns_lock still protects the tree entry.
> The unregister path then cannot drop the last reference until termination
> has finished using the socket.

The race description and fix look correct to me. Taking the socket
reference before dropping *conns_lock* closes the window where a
concurrent close can free the socket before sock_hold().

> 
> Fixes: 69318b5215f2 ("net/smc: improve abnormal termination locking")
> Cc: [email protected]
> Signed-off-by: Xuanqiang Luo <[email protected]>

Reviewed-by: Mahanta Jambigi <[email protected]>

> ---
>  net/smc/smc_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index cf6b620fef05f..b4208cb186c5e 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -1572,10 +1572,10 @@ static void __smc_lgr_terminate(struct smc_link_group *lgr, bool soft)
>  	read_lock_bh(&lgr->conns_lock);
>  	node = rb_first(&lgr->conns_all);
>  	while (node) {
> -		read_unlock_bh(&lgr->conns_lock);
>  		conn = rb_entry(node, struct smc_connection, alert_node);
>  		smc = container_of(conn, struct smc_sock, conn);
>  		sock_hold(&smc->sk); /* sock_put below */
> +		read_unlock_bh(&lgr->conns_lock);
>  		lock_sock(&smc->sk);
>  		smc_conn_kill(conn, soft);
>  		release_sock(&smc->sk);