Re: [PATCH net v5 2/3] net/smc: bound the receive length to the RMB in smc_rx_recvmsg()

[Sidraya Jayagond]

On 24/07/26 5:53 am, Bryam Vargas via B4 Relay wrote:
> From: Bryam Vargas <hexlabsecurity@xxxxxxxxx>
> 
> conn->bytes_to_rcv is accumulated in the receive tasklet from the
> peer's wire-controlled producer cursor via smc_curs_diff(), whose
> differing-wrap branch can exceed rmb_desc->len; a forged cursor drives
> bytes_to_rcv past the RMB, and over many CDC messages overflows the
> signed counter negative. smc_rx_recvmsg() reads it as the readable
> length and does a wrap-around copy whose second chunk is not re-bounded
> to rmb_desc->len, reading past the RMB into adjacent kernel memory and
> disclosing it to the peer. The nearby readable >= rmb_desc->len test
> only feeds SMC_STAT_RMB_RX_FULL on a separate earlier read; it does not
> bound the copy.
> 
> Bound the readable length to rmb_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 <= bytes_to_rcv <= rmb_desc->len invariant where it is race-free
> against the producer update in the tasklet; conforming peers are
> unaffected.
> 
> Fixes: 952310ccf2d8 ("smc: receive data from RMBE")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Bryam Vargas <hexlabsecurity@xxxxxxxxx>
> Reviewed-by: Dust Li <dust.li@xxxxxxxxxxxxxxxxx>
> ---
>  net/smc/smc_rx.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/net/smc/smc_rx.c b/net/smc/smc_rx.c
> index c1d9b923938d..f461cf10b085 100644
> --- a/net/smc/smc_rx.c
> +++ b/net/smc/smc_rx.c
> @@ -442,6 +442,18 @@ int smc_rx_recvmsg(struct smc_sock *smc, struct msghdr *msg,
>  		/* initialize variables for 1st iteration of subsequent loop */
>  		/* could be just 1 byte, even after waiting on data above */
>  		readable = smc_rx_data_available(conn, peeked_bytes);
> +		/* bytes_to_rcv is accumulated from the peer's wire-controlled
> +		 * producer cursor; a forged cursor can drive it past the RMB,
> +		 * 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
> +		 * RMB in either case so the wrap-around copy cannot run past
> +		 * rmb_desc->len.  This enforces the documented
> +		 * 0 <= bytes_to_rcv <= rmb_desc->len invariant at the consumer,
> +		 * race-free against the producer update in the receive tasklet.
> +		 */
> +		if (readable < 0 || readable > conn->rmb_desc->len)
> +			readable = conn->rmb_desc->len;
>  		splbytes = atomic_read(&conn->splice_pending);
>  		if (!readable || (msg && splbytes)) {
>  			if (splbytes)
> 

Reviewed-by: Sidraya Jayagond <sidraya@xxxxxxxxxxxxx>