Re: [PATCH net v2 2/2] net/smc: do not dereference an unset send buffer on the SMC-D teardown path

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch prevents a NULL pointer dereference but leaves a Use-After-Free (UAF) vulnerability in `smc_tx_prepared_sends()` by locklessly dereferencing `sndbuf_desc->len` without object lifetime protection.
--

--- Patch [2]: [PATCH net v2 2/2] net/smc: do not dereference an unset send buffer on the SMC-D teardown path ---
commit 08550d7abeeecb4d3f4b5d472d83dee49a54e0ba
Author: Bryam Vargas <[email protected]>

net/smc: do not dereference an unset send buffer on the SMC-D teardown path

This patch addresses a NULL pointer dereference during SMC-D teardown by
using READ_ONCE to cache the send buffer descriptor and checking it for
NULL. It ensures that lockless evaluations inside sk_wait_event() do not
fault when the descriptor is cleared concurrently.

> diff --git a/net/smc/smc_tx.h b/net/smc/smc_tx.h
> index a59f370b8b432..610a945aefd69 100644
> --- a/net/smc/smc_tx.h
> +++ b/net/smc/smc_tx.h
> @@ -20,11 +20,15 @@
>  
>  static inline int smc_tx_prepared_sends(struct smc_connection *conn)
>  {
> +	struct smc_buf_desc *sndbuf_desc = READ_ONCE(conn->sndbuf_desc);
>  	union smc_host_cursor sent, prep;
>  
> +	if (!sndbuf_desc)
> +		return 0;
> +
>  	smc_curs_copy(&sent, &conn->tx_curs_sent, conn);
>  	smc_curs_copy(&prep, &conn->tx_curs_prep, conn);
> -	return smc_curs_diff(conn->sndbuf_desc->len, &sent, &prep);

[Severity: High]
This is a pre-existing issue, but does this lockless dereference leave a
use-after-free vulnerability?

If CPU1 evaluates this condition via sk_wait_event() in
smc_close_stream_wait(), it explicitly drops the socket lock. CPU1 might
cache the pointer here:

CPU1:
smc_tx_prepared_sends()
    struct smc_buf_desc *sndbuf_desc = READ_ONCE(conn->sndbuf_desc);
    if (!sndbuf_desc)
        // bypasses this check

If CPU1 is preempted and CPU2 concurrently terminates the connection, it
acquires the socket lock and could call smcd_buf_detach() to free the buffer:

CPU2:
net/smc/smc_core.c:smcd_buf_detach()
    buf_desc = conn->sndbuf_desc;
    conn->sndbuf_desc = NULL;
    kfree(buf_desc);

Wouldn't CPU1 then dereference the freed descriptor without any object
lifetime protection when it eventually resumes, calls smc_curs_diff(), and
accesses sndbuf_desc->len?

> +	return smc_curs_diff(sndbuf_desc->len, &sent, &prep);
>  }
>  
>  void smc_tx_pending(struct smc_connection *conn);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2