Re: [PATCH net v7 3/4] net/smc: bound the send length to the send buffer in smc_tx_sendmsg()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] smc: flow control bypass and data corruption on negative sndbuf_space
--

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

net/smc: bound the send length to the send buffer in smc_tx_sendmsg()

This commit limits the writespace to the size of the send buffer descriptor
to prevent writing past the send buffer when sndbuf_space exceeds its
bounds. It explicitly maps negative writespace values to the full buffer
length.

Cc: stable@vger.kernel.org

> diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c
> --- a/net/smc/smc_tx.c
> +++ b/net/smc/smc_tx.c

[ ... ]

> @@ -233,6 +233,14 @@ int smc_tx_sendmsg(struct smc_sock *smc, struct msghdr *msg, size_t len)
>  		/* initialize variables for 1st iteration of subsequent loop */
>  		/* could be just 1 byte, even after smc_tx_wait above */
>  		writespace = atomic_read(&conn->sndbuf_space);
> +		/* sndbuf_space can be out of range: late completions of an
> +		 * old SMC-R link credit it again after a failover, and with
> +		 * SMC-D DMB merge it follows the peer's consumer cursor.
> +		 * Bound writespace, which sizes the copy; a negative value
> +		 * would become huge in the size_t min_t() below.
> +		 */
> +		if (writespace < 0 || writespace > conn->sndbuf_desc->len)
> +			writespace = conn->sndbuf_desc->len;

[Severity: High]
Does this mapping of negative writespace to the full buffer length bypass
flow control and overwrite unacknowledged data?

When sndbuf_space becomes negative due to over-credits, the earlier check
in smc_tx_sendmsg() for !atomic_read(&conn->sndbuf_space) will evaluate to
false, which completely skips smc_tx_wait().

By then forcing the negative writespace to conn->sndbuf_desc->len, the loop
will copy up to the full buffer length over unacknowledged data.

The subsequent atomic_sub() later in this loop will drive the space even
further negative, creating an unthrottled copy loop until the user message
is exhausted.

Could we handle negative values by treating them as zero to properly wait
for space, rather than mapping them to the maximum buffer length?

>  		/* not more than what user space asked for */
>  		copylen = min_t(size_t, send_remaining, writespace);
>  		/* determine start of sndbuf */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-b4-disp-f345196c-v7-0-ef3dddfbfa68@proton.me?part=3