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

[netdev-bot+sashiko@kernel.org]

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

Critical: 1 · High: 1 · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Critical] The fix is incomplete: smc_rx_recvmsg() (net/smc/smc_rx.c)
  can still read past the RMB and copy that kernel memory to userspace.
- [High] smc_rx_recvmsg() (net/smc/smc_rx.c:444-456) can see a negative
  `readable` from a purely local race, with no forged peer.
- [Low] net/smc: invariant violation by subtracting from negative
  bytes_to_rcv

--

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

This patch limits the readable length in smc_rx_recvmsg() to
rmb_desc->len. It also maps negative values to rmb_desc->len. The goal is
to stop a forged peer producer cursor from pushing the wrap-around copy
past the end of the RMB.

> 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: Low]
The new comment says this check "enforces the documented
0 <= bytes_to_rcv <= rmb_desc->len invariant at the consumer". Does it?

The clamp only changes the local readable copy. conn->bytes_to_rcv
itself is left as it was. Further down, the non-peek path still does:

	atomic_sub(copylen, &conn->bytes_to_rcv);
	/* guarantee 0 <= bytes_to_rcv <= rmb_desc->len */

Suppose a forged producer cursor has already pushed bytes_to_rcv
negative. readable is then mapped to rmb_desc->len, so copylen can be
up to a full ring. That copylen is then subtracted from a counter that
is already below zero. On every pass the counter moves further from
the invariant, not back toward it.

smc_rx_data_available() keeps returning a non-zero negative value.
So every later recvmsg() call goes straight to the copy label and is
clamped to a full ring again. It hands that ring to user space and
advances the consumer cursor through smc_rx_update_consumer().

The connection never recovers from the bad state. The patch just keeps
re-clamping it, and the comment next to the atomic_sub() becomes
false.

Either the commit message and the new comment should stop claiming the
invariant is enforced, or the bad state should be handled. Would it be
better to detect the out-of-range counter and abort the connection, or
mark it out_of_sync, rather than keep reading from it? The detection
could go in smc_cdc_msg_recv_action() when diff_prod is added, or
here.

[Severity: Critical]
The commit message says that with this check "the copy can never exceed
the ring". Is that the case?

The check limits the length copied in this pass. The copy start offset,
though, comes separately from the running peeked_bytes and
splice_pending values:

net/smc/smc_rx.c:smc_rx_recvmsg() {
    ...
	smc_curs_copy(&cons, &conn->local_tx_ctrl.cons, conn);
	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);
    ...
}

smc_curs_add() subtracts size only once:

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

So once peeked_bytes reaches 2 * len - cons.count, cons.count stays at or
above rmb_desc->len.

The chunk calculation then subtracts a u32 from an int:

	chunk_len = min_t(size_t, copylen, conn->rmb_desc->len -
			  cons.count);
	chunk_len_sum = chunk_len;
	chunk_off = cons.count;

The result wraps as unsigned. That gives chunk_len = copylen (up to
rmb_desc->len) and chunk_off >= rmb_desc->len. The call
memcpy_to_msg(msg, rcvbuf_base + chunk_off, chunk_len) would then copy
memory beyond the RMB into the user buffer.

On the splice path, smc_rx_splice() would set partial[0].offset past the
buffer. For is_vm RMBs it would call vmalloc_to_page() on an address
outside the vmalloc area.

For example, take RMB length L, a starting consumer count c with
0 < c < L, bytes_to_rcv >= 3L, and recv(MSG_PEEK | MSG_WAITALL) with a
buffer larger than about 2L:

  pass 1: copies L bytes, peeked_bytes = L
  pass 2: cons = c + L wraps to c, peeked_bytes = 2L
  pass 3: cons = c + 2L, single subtraction leaves c + L >= L,
          chunk_len = min(L, (u32)(L - (c + L))) = L,
          chunk_off = c + L, so [c + L, c + 2L) is read, entirely
          past the RMB

Can a peer push bytes_to_rcv to 3L or more? smc_cdc_msg_recv_action()
still adds diff_prod with no limit:

	atomic_add(diff_prod, &conn->bytes_to_rcv);

The differing-wrap branch of smc_curs_diff() returns
(size - old->count) + new->count. That is close to 2 * len per CDC
message.

The earlier patch in this series, "net/smc: bound the wire-controlled
producer cursor to the RMB", limits count but not wrap. A peer that bumps
wrap on every message can keep growing the counter.

The bad offset also looks reachable without a malicious peer, through the
local MSG_PEEK race described in the next comment.

The later patch in the series, "net/smc: bound the send length to the
send buffer in smc_tx_sendmsg()", only touches net/smc/smc_tx.c, so this
does not seem to be handled anywhere else in the series.

Could bytes_to_rcv be limited at the producer in the tasklet instead?
Another option is to compute the available length as
min(bytes_to_rcv, len) minus peeked_bytes / splbytes, and treat a
negative result as 0.

[Severity: High]
Can readable go negative here from a purely local race, with no forged
peer? smc_rx_data_available() returns:

	return atomic_read(&conn->bytes_to_rcv) - peeked;

Consider two threads on the same AF_SMC socket:

Thread A
recv(MSG_PEEK | MSG_WAITALL, large buffer)
  peeks all N bytes, peeked_bytes = N, read_done < target
  smc_rx_wait()
    sk_wait_event()
      release_sock()

Thread B
recv()
  lock_sock()
  consumes the N bytes
    atomic_sub(copylen, &conn->bytes_to_rcv)  -> 0
    smc_rx_update_consumer()  -> local_tx_ctrl.cons advanced by N

Thread A then wakes on SO_RCVTIMEO, a signal, or new data M < N. The
fcrit() check sees bytes_to_rcv - N < 0, which is non-zero. So
smc_rx_wait() returns and thread A jumps to copy with
readable = M - N < 0.

With this patch, that negative value becomes rmb_desc->len (a full ring)
instead of "no data".

Thread A rebuilds cons from the shared local_tx_ctrl.cons, which thread B
has already advanced, plus the stale peeked_bytes. It then copies a full
ring of bytes that are not valid stream data: either data already
consumed, or space the peer has not written yet. RMB contents are not
cleared when buffers are reused, so these bytes can be left over from
earlier traffic.

Each later pass computes a more negative readable, which is again mapped
to rmb_desc->len, so peeked_bytes grows by a full ring per pass. Within
two or three passes, smc_curs_add() leaves cons.count >= len, and
memcpy_to_msg() reads past the RMB as described in the previous comment.

The new comment says the check is "race-free against the producer update
in the receive tasklet", but this race is between two consumers. TCP
handles the same situation by resetting peek_seq to copied_seq.

Should a negative readable be treated as 0 (no data), or should the peek
state be reset, rather than mapping it to rmb_desc->len?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008-b4-disp-f7cd3d96-v6-0-fba616c0920c%40proton.me