Re: [PATCH net v6 1/3] net/smc: bound the wire-controlled producer cursor to the RMB

[netdev-bot+sashiko]

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

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

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).

- [High] Incomplete fix: the patch caps only the count of the wire-
  controlled producer cursor at rmb_desc->len.

--

net/smc: bound the wire-controlled producer cursor to the RMB

When CDC messages are converted to host order, this patch clamps the count
of the peer's producer cursor to rmb_desc->len. It does this for both SMC-R
and SMC-D. The consumer cursor is left unbounded because it indexes the
peer's RMB.

> diff --git a/net/smc/smc_cdc.h b/net/smc/smc_cdc.h
> index 696cc11f2303b..ca76ef6303566 100644
> --- a/net/smc/smc_cdc.h
> +++ b/net/smc/smc_cdc.h

[ ... ]

> @@ -235,6 +236,15 @@ static inline void smc_cdc_cursor_to_host(union smc_host_cursor *local,
>  	if ((old.wrap == temp.wrap) &&
>  	    (old.count > temp.count))
>  		return;
> +	/* The peer producer cursor is wire-controlled and is later used as a
> +	 * raw index into our RMB by the urgent path; bound its count to the
> +	 * RMB.  max_count == 0 leaves the consumer cursor unbounded here: it
> +	 * indexes the peer's RMB (bounded by peer_rmbe_size, not our
> +	 * rmb_desc->len), so clamping it to rmb_desc->len would under-credit
> +	 * peer_rmbe_space and stall transmit to peers with a larger RMB.
> +	 */
> +	if (max_count && temp.count > max_count)
> +		temp.count = max_count;
>  	smc_curs_copy(local, &temp, conn);
>  }

[Severity: High]

Does clamping only the count keep the producer cursor inside the RMB? The
checks above only reject a cursor that moves backwards. A peer can still
move prod.wrap forward by any amount in one message. The only gate before
this is the seqno check in smc_cdc_rx_handler(), and the peer sets seqno.

smcd_cdc_msg_to_host() has the same count-only clamp. It also takes
peer->prod.wrap with no check at all.

The new cursor goes straight into bytes_to_rcv:

net/smc/smc_cdc.c:smc_cdc_msg_recv_action() {
    ...
	diff_prod = smc_curs_diff(conn->rmb_desc->len, &prod_old,
				  &conn->local_rx_ctrl.prod);
	if (diff_prod) {
		...
		atomic_add(diff_prod, &conn->bytes_to_rcv);
		/* guarantee 0 <= bytes_to_rcv <= rmb_desc->len */
    ...
}

When the wraps differ, smc_curs_diff() returns
(size - old->count) + new->count. Even with new->count clamped to len, one
message can still add up to 2 * len. For example:

    prod (0, 0) -> (1, len)   adds 2 * len
    then (2, len), (3, len)   each adds another len

Nothing enforces the "guarantee" in that comment.

With only this patch applied, smc_rx_recvmsg() computes copylen as
min(read_remaining, readable), where readable comes from bytes_to_rcv. The
second chunk is then read from offset 0:

	chunk_len = copylen - chunk_len; /* remainder */
	...
	chunk_off = 0; /* modulo offset in recv ring buffer */

Can that remainder be larger than rmb_desc->len? If so, memcpy_to_msg() or
smc_rx_splice() would read past rcvbuf_base + len and copy kernel memory to
userspace.

A later patch in this series, "net/smc: bound the receive length to the
RMB in smc_rx_recvmsg()", caps readable at rmb_desc->len on each pass. That
closes the direct overrun in the second chunk. However, bytes_to_rcv can
still be inflated, and smc_rx_recvmsg() builds its local cursor like this:

	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. With MSG_PEEK | MSG_WAITALL and a
large read length, peeked_bytes can reach 2 * len after two passes. That
leaves cons.count >= len, and then:

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

The subtraction is done in u32 and wraps, so chunk_len becomes copylen.
memcpy_to_msg() then reads from rcvbuf_base + cons.count, which is
completely past the end of the RMB. Repeated splice() calls that let
splice_pending grow without draining the pipe seem to hit the same path.

Would it work better to reject any producer advance larger than the free
RMB space (for example, bytes_to_rcv + diff_prod > rmb_desc->len) and abort
the connection, instead of clamping the count here?

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