Re: [PATCH net] net/smc: guard the CDC receive path against an unset RMB

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The newly added check for `!conn->rmb_desc` lacks memory barriers, resulting in a data race. The writer (`__smc_buf_create()`) assigns `conn->rmb_desc = buf_desc` without a write barrier (e.g., `smp_store_release()`) and outside of any lock shared with the reader. Concurrently, the readers (`smc_cdc_rx_handler()` and `smcd_cdc_rx_tsklet()`) perform a plain load of `conn->rmb_desc` without a read barrier (e.g., `smp_load_acquire()` or `READ_ONCE()`). On weakly-ordered architectures, the reader can observe a non-NULL pointer but see uninitialized contents of `buf_desc` (like `cpu_addr` and `len`), causing memory corruption or a NULL pointer dereference.
--

--- Patch [1]: [PATCH net] net/smc: guard the CDC receive path against an unset RMB ---
commit d9db6cbb0ec5bacaa514e8123311e01c799a00da
Author: Bryam Vargas <hexlabsecurity@proton.me>
Subject: net/smc: guard the CDC receive path against an unset RMB

This commit guards against a NULL pointer dereference in SMC CDC receive
handlers when a connection's RMB is not yet allocated. It bails out of
the handlers if conn->rmb_desc is NULL.

Link: https://sashiko.dev/#/patchset/20260705-b4-disp-28a1bbca-v4-1-be089b98acc6@proton.me?part=1

> diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
> index 32d6d03df3214..d60f68facbc34 100644
> --- a/net/smc/smc_cdc.c
> +++ b/net/smc/smc_cdc.c
> @@ -446,7 +446,7 @@ static void smcd_cdc_rx_tsklet(struct tasklet_struct *t)
>  	struct smcd_cdc_msg cdc;
>  	struct smc_sock *smc;
>  
> -	if (!conn || conn->killed)
> +	if (!conn || conn->killed || !conn->rmb_desc)

[Severity: High]
Is there a potential data race with __smc_buf_create() here?

The writer __smc_buf_create() assigns conn->rmb_desc = buf_desc without a
write barrier (e.g., smp_store_release()), and outside of any lock shared
with this reader.

If this plain load observes a non-NULL pointer, could weakly-ordered
architectures see uninitialized contents of buf_desc (like cpu_addr and len)?

Would it be safer to use smp_load_acquire() here and smp_store_release() on
the writer side?

>  		return;
>  
>  	data_cdc = (struct smcd_cdc_msg *)conn->rmb_desc->cpu_addr;

[ ... ]

> @@ -483,7 +483,7 @@ static void smc_cdc_rx_handler(struct ib_wc *wc, void *buf)
>  	lgr = smc_get_lgr(link);
>  	read_lock_bh(&lgr->conns_lock);
>  	conn = smc_lgr_find_conn(ntohl(cdc->token), lgr);
> -	if (!conn || conn->out_of_sync) {
> +	if (!conn || conn->out_of_sync || !conn->rmb_desc) {

[Severity: High]
Does this check need the same read barrier protection?

Even though this is inside read_lock_bh(&lgr->conns_lock), the writer
__smc_buf_create() does not hold this lock when assigning the descriptor.

>  		read_unlock_bh(&lgr->conns_lock);
>  		return;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711-b4-disp-c36a9798-v1-1-340b0c6053fb@proton.me?part=1