Re: [PATCH net v2] net/smc: validate peer CDC cursor against RMBE size before accepting it

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The patch bounds the cursor advance of a single CDC message but fails to limit the cumulative accumulation of `bytes_to_rcv` across multiple messages. An attacker can repeatedly advance the cursor to inflate `bytes_to_rcv` beyond the buffer limits, resulting in the same out-of-bounds kernel memory read the patch attempted to fix.
- [Critical] The exact same remote kernel memory disclosure vulnerability remains unfixed for SMC-D connections, as `smcd_cdc_msg_to_host()` skips cursor validation.
- [High] Incoming CDC messages can be processed before connection buffers are allocated, leading to a NULL pointer dereference on `conn->rmb_desc`.
- [High] On 32-bit systems, reading the 64-bit cursor from the SMC-D shared DMA memory is protected only by a local OS spinlock, failing to prevent torn reads against concurrent hardware modifications by the peer.
--

--- Patch [1]: [PATCH net v2] net/smc: validate peer CDC cursor against RMBE size before accepting it ---
commit 7b6c0d41f37fa23577369a83f25810b1711838f7
Author: Ibrahim Hashimov <security@auditcode.ai>

net/smc: validate peer CDC cursor against RMBE size before accepting it

This commit adds boundary checks to SMC-R CDC cursors at ingress to prevent
out-of-bounds reads. It introduces a size parameter to
smc_cdc_cursor_to_host() to reject cursors that exceed the buffer limits or
advance more than one buffer size.

> diff --git a/net/smc/smc_cdc.h b/net/smc/smc_cdc.h
> index 696cc11f2303b..b0ae4d43b0171 100644
> --- a/net/smc/smc_cdc.h
> +++ b/net/smc/smc_cdc.h

[ ... ]

> @@ -235,6 +236,21 @@ static inline void smc_cdc_cursor_to_host(union smc_host_cursor *local,
>  	if ((old.wrap == temp.wrap) &&
>  	    (old.count > temp.count))
>  		return;
> +	/* count is an offset into the RMBE and must always stay inside
> +	 * it (see smc_curs_add()); the peer is untrusted, so reject an
> +	 * out-of-range wire cursor the same way an out-of-order one is
> +	 * already rejected above, instead of letting it drive
> +	 * bytes_to_rcv / the urgent-byte offset past the buffer end
> +	 */
> +	if (temp.count >= size)
> +		return;
> +	/* Bound the advance too: a wrap increment with temp.count above
> +	 * old.count makes smc_curs_diff() report more than one bufferful,
> +	 * which would inflate bytes_to_rcv / peer_rmbe_space past the buffer
> +	 * in smc_cdc_msg_recv_action(). Reject such a cursor.
> +	 */
> +	if (smc_curs_diff(size, &old, &temp) > size)
> +		return;

[Severity: Critical]
This is a pre-existing issue, but does this security check miss the stateful
cumulative buildup of the cursor?

By only validating single-message advances, could an attacker repeatedly
advance the cursor to inflate bytes_to_rcv beyond the buffer limits?

This could result in an out-of-bounds read if a malicious peer sends multiple
valid CDC messages that each advance the cursor within the single-message
bounds. These advances would continually add to bytes_to_rcv without clamping
to rmb_desc->len in smc_cdc_msg_recv_action():

smc_cdc_msg_recv_action() {
    ...
    atomic_add(diff_prod, &conn->bytes_to_rcv);
    ...
}

A subsequent user-space read via smc_rx_recvmsg() could then use this
inflated bytes_to_rcv value to read past the boundaries of the buffer:

smc_rx_recvmsg() {
    ...
    chunk_len = copylen - chunk_len; /* remainder */
    chunk_len_sum += chunk_len;
    chunk_off = 0; /* modulo offset in recv ring buffer */
    ...
}

>  	smc_curs_copy(local, &temp, conn);
>  }
>  
> @@ -246,8 +262,10 @@ static inline void smcr_cdc_msg_to_host(struct smc_host_cdc_msg *local,
>  	local->len = peer->len;
>  	local->seqno = ntohs(peer->seqno);
>  	local->token = ntohl(peer->token);
> -	smc_cdc_cursor_to_host(&local->prod, &peer->prod, conn);
> -	smc_cdc_cursor_to_host(&local->cons, &peer->cons, conn);
> +	smc_cdc_cursor_to_host(&local->prod, &peer->prod,
> +			       conn->rmb_desc->len, conn);

[Severity: High]
This isn't a bug introduced by this patch, but could this unconditional
dereference of conn->rmb_desc->len cause a kernel panic if rmb_desc is NULL?

If an attacker on an existing link group anticipates the predictable
alert_token_local and sends a matching CDC message right after
smc_lgr_register_conn() exposes the connection, the RX tasklet might
access conn->rmb_desc->len in smcr_cdc_msg_to_host() before
smc_buf_create() has allocated the buffer descriptors.

[Severity: Critical]
This is a pre-existing issue, but are SMC-D connections vulnerable to the same
remote kernel memory disclosure?

In smcd_cdc_msg_to_host(), the untrusted SMC-D cursor values appear to be
directly copied without applying the boundary checks introduced here for
SMC-R:

smcd_cdc_msg_to_host() {
    ...
    temp.wrap = peer->prod.wrap;
    temp.count = peer->prod.count;
    smc_curs_copy(&local->prod, &temp, conn);
    ...
}

Could a malicious peer over SMC-D (ISM) send a CDC message with an inflated
cursor, bypassing standard sanity checks and calculating arbitrary
differentials via smc_curs_diff() to inflate bytes_to_rcv?

[Severity: High]
This is a pre-existing issue, but does the spinlock in smcd_curs_copy()
actually protect against concurrent hardware DMA or cross-LPAR shared memory
modifications?

smcd_curs_copy() {
    ...
    spin_lock_irqsave(&conn->acurs_lock, flags);
    tgt->acurs = src->acurs;
    spin_unlock_irqrestore(&conn->acurs_lock, flags);
    ...
}

On 32-bit systems (where KERNEL_HAS_ATOMIC64 is not defined), reading the
64-bit cursor from the SMC-D shared DMA memory is protected by a local OS
spinlock.

Since the spinlock does not serialize with the peer's DMA/memory writes,
could this lead to a torn read of the 64-bit cursor if the compiler emits two
32-bit loads that interleave with the peer's writes?

> +	smc_cdc_cursor_to_host(&local->cons, &peer->cons,
> +			       conn->peer_rmbe_size, conn);
>  	local->prod_flags = peer->prod_flags;
>  	local->conn_state_flags = peer->conn_state_flags;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722101758.37817-1-security@auditcode.ai?part=1