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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net/smc: OOB read in smc_rx_recvmsg due to MSG_PEEK/splice offset accumulation
- [Low] net/smc: invariant violation by subtracting from negative bytes_to_rcv
--

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

net/smc: bound the receive length to the RMB in smc_rx_recvmsg()

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.

> diff --git a/net/smc/smc_rx.c b/net/smc/smc_rx.c
> index 197fddc6271f0..f781d7229185c 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;

[Severity: High]
Does this code introduce an out-of-bounds read due to MSG_PEEK or splice
offset accumulation?

If bytes_to_rcv is inflated by a forged cursor to a value much larger than
rmb_desc->len, readable is clamped here. However, MSG_PEEK accumulates
peeked_bytes and splice() accumulates splbytes across loops.

These accumulated values are added to the local cursor using smc_curs_add():

net/smc/smc_rx.c:smc_rx_recvmsg() {
	...
		if ((flags & MSG_PEEK) && peeked_bytes)
			smc_curs_add(conn->rmb_desc->len, &cons, peeked_bytes);
		/* subsequent splice() calls pick up where previous left */
		if (splbytes)
			smc_curs_add(conn->rmb_desc->len, &cons, splbytes);
	...
}

Because smc_curs_add() only performs a single wrap-around subtraction:

net/smc/smc_cdc.h:smc_curs_add() {
	...
	curs->count += value;
	if (curs->count >= size) {
		curs->wrap++;
		curs->count -= size;
	}
	...
}

If peeked_bytes or splbytes exceeds 2 * rmb_desc->len, cons.count
fails to wrap completely and remains >= rmb_desc->len.

This causes the bounds check calculation to underflow into a huge size_t
value, bypassing the checks and copying out of bounds:

net/smc/smc_rx.c:smc_rx_recvmsg() {
	...
		chunk_len = min_t(size_t, copylen, conn->rmb_desc->len -
				  cons.count);
	...
}

[Severity: Low]
Does this clamp completely repair the invariant for bytes_to_rcv?

While readable is clamped to a positive value here, atomic_sub() is later
performed on the underlying bytes_to_rcv counter using copylen (which
is derived from readable):

net/smc/smc_rx.c:smc_rx_recvmsg() {
	...
		/* update cursors */
		if (!(flags & MSG_PEEK)) {
			/* increased in recv tasklet smc_cdc_msg_rcv() */
			smp_mb__before_atomic();
			atomic_sub(copylen, &conn->bytes_to_rcv);
			/* guarantee 0 <= bytes_to_rcv <= rmb_desc->len */
	...
}

If bytes_to_rcv was already driven negative by a forged cursor,
subtracting a positive copylen from it drives the atomic counter further
negative. This seems to violate the 0 <= bytes_to_rcv <= rmb_desc->len
invariant that the patch explicitly claims to guarantee.

>  		splbytes = atomic_read(&conn->splice_pending);
>  		if (!readable || (msg && splbytes)) {
>  			if (splbytes)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-b4-disp-f7cd3d96-v6-0-fba616c0920c@proton.me?part=2