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

[Sidraya Jayagond]

On 24/07/26 5:53 am, Bryam Vargas via B4 Relay wrote:
> From: Bryam Vargas <hexlabsecurity@xxxxxxxxx>
> 
> On the SMC-D DMB-merge (nocopy) path, smc_cdc_msg_recv_action()
> advances conn->sndbuf_space from the peer's wire-controlled consumer
> cursor via smc_curs_diff(), which can return more than sndbuf_desc->len;
> a forged cursor drives sndbuf_space past the send buffer, and over many
> CDC messages overflows the signed counter negative. smc_tx_sendmsg()
> reads it as the write space and does a wrap-around copy whose second
> chunk is not re-bounded to sndbuf_desc->len, spilling the local
> sender's outbound data past the send buffer at a peer-controlled
> length: a heap out-of-bounds write. The nearby len > sndbuf_desc->len
> test only feeds SMC_STAT_RMB_TX_SIZE_SMALL on the user length; it does
> not bound the copy.
> 
> Bound the write space to sndbuf_desc->len at the consumer, treating a
> negative (sign-overflowed) value as out of range too, so the copy can
> never exceed the ring. This enforces the documented
> 0 <= sndbuf_space <= sndbuf_desc->len invariant where it is race-free
> against the CDC tasklet; conforming peers are unaffected.
> 
> Fixes: cc0ab806fc52 ("net/smc: adapt cursor update when sndbuf and peer DMB are merged")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Bryam Vargas <hexlabsecurity@xxxxxxxxx>
> Reviewed-by: Dust Li <dust.li@xxxxxxxxxxxxxxxxx>
> ---
>  net/smc/smc_tx.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c
> index 3144b4b1fe29..5916f02060fb 100644
> --- a/net/smc/smc_tx.c
> +++ b/net/smc/smc_tx.c
> @@ -233,6 +233,19 @@ 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 is advanced from the peer's wire-controlled
> +		 * consumer cursor on the SMC-D DMB-merge path; a forged cursor
> +		 * can inflate it past the send buffer, or overflow the signed
> +		 * accumulator to a negative value across many CDC messages
> +		 * (which a plain "> len" check would miss before the size_t
> +		 * cast below turns it huge).  Bound it to the send buffer in
> +		 * either case so the wrap-around write cannot run past
> +		 * sndbuf_desc->len.  This enforces the documented
> +		 * 0 <= sndbuf_space <= sndbuf_desc->len invariant at the
> +		 * producer, race-free against the CDC tasklet.
> +		 */
> +		if (writespace < 0 || writespace > conn->sndbuf_desc->len)
> +			writespace = conn->sndbuf_desc->len;
>  		/* not more than what user space asked for */
>  		copylen = min_t(size_t, send_remaining, writespace);
>  		/* determine start of sndbuf */
> 

Reviewed-by: Sidraya Jayagond <sidraya@xxxxxxxxxxxxx>