Re: [PATCH] net/smc: bound sndbuf_space on the SMC-D DMB-merge receive path

[sashiko-bot@kernel.org]

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

New issues:
- [High] TOCTOU race condition in `sndbuf_space` clamp allows heap out-of-bounds write

Pre-existing issues:
- [Critical] Unchecked `bytes_to_rcv` inflation leading to heap out-of-bounds read
- [Critical] Signed integer overflow bypasses the `sndbuf_space` clamp check
--

commit e92859ab3e83950d546c2c14959ddfecc322ce3e
Author: Bryam Vargas <hexlabsecurity@proton.me>

net/smc: bound sndbuf_space on the SMC-D DMB-merge receive path

This commit modifies the receive path to clamp sndbuf_space after calculating
the cursor difference. It uses an atomic_add followed by a conditional
atomic_set to enforce the bounds. The intent is to prevent sndbuf_space from
exceeding the send buffer length.

> Enforce the documented invariant after the cursor-driven atomic_add(), as
> the SMC-D receive path already does for bytes_to_rcv.

[Severity: Critical]
This is a pre-existing issue, but does the receive path actually enforce
invariants for bytes_to_rcv as claimed here?

In smc_cdc_msg_recv_action(), diff_prod is calculated from an unvalidated
peer cursor and atomically added to conn->bytes_to_rcv without any clamping:

net/smc/smc_cdc.c:smc_cdc_msg_recv_action() {
    ...
    atomic_add(diff_prod, &conn->bytes_to_rcv);
    /* guarantee 0 <= bytes_to_rcv <= rmb_desc->len */
    smp_mb__after_atomic();
    ...
}

If the cdc->prod cursor causes an inflated bytes_to_rcv value, and
smc_rx_recvmsg() reads this value:

net/smc/smc_rx.c:smc_rx_recvmsg() {
    ...
    copylen = min_t(size_t, read_remaining, readable);
    ...
}

Could the length of the second chunk exceed the buffer size, causing
memcpy_to_msg() or splice_to_pipe() to read memory out of bounds?

> diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
> index 619b3bab38248..cf8d65407ea5d 100644
> --- a/net/smc/smc_cdc.c
> +++ b/net/smc/smc_cdc.c
> @@ -365,6 +365,10 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
>  			smp_mb__before_atomic();
>  			atomic_add(diff_tx, &conn->sndbuf_space);
>  			/* guarantee 0 <= sndbuf_space <= sndbuf_desc->len */
> +			if (atomic_read(&conn->sndbuf_space) >
> +			    conn->sndbuf_desc->len)
> +				atomic_set(&conn->sndbuf_space,
> +					   conn->sndbuf_desc->len);

[Severity: High]
Can this non-atomic clamping sequence create a race condition?

If smc_tx_sendmsg() runs concurrently in process context, could it locklessly
read the temporarily inflated sndbuf_space between the atomic_add() and
atomic_set() calls?

net/smc/smc_tx.c:smc_tx_sendmsg() {
    ...
    writespace = atomic_read(&conn->sndbuf_space);
    ...
}

Using this transiently inflated value, could the second loop iteration in
smc_tx_sendmsg() write memory out of bounds past the send buffer?

[Severity: Critical]
This is a pre-existing issue, but does this signed comparison fail to catch
negative wrapped values caused by signed integer overflow?

If a forged cursor causes smc_curs_diff() to return a very large value,
and this is added via atomic_add(), the 32-bit signed integer could overflow
into a negative value. The check would then compare this negative value
against the positive sndbuf_desc->len, evaluate to false, and bypass the
clamp entirely.

In smc_tx_sendmsg(), if this negative writespace is cast to size_t:

net/smc/smc_tx.c:smc_tx_sendmsg() {
    ...
    copylen = min_t(size_t, send_remaining, writespace);
    ...
}

Would this result in an unexpectedly large unsigned value, causing the
second loop iteration to write memory out of bounds?

>  			smp_mb__after_atomic();
>  			smc_curs_copy(&conn->tx_curs_fin,
>  				      &conn->local_rx_ctrl.cons, conn);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610090928.192177-1-hexlabsecurity@proton.me?part=1