Re: [PATCH net v4 0/3] net/smc: bound wire-controlled CDC cursors against the local buffers

[Dust Li <dust.li@linux.alibaba.com>]

On 2026-07-05 02:54:04, Bryam Vargas via B4 Relay wrote:
>A peer's CDC producer/consumer cursors are copied from the wire and used,
>without an upper bound against the local buffers, as (a) a raw index into the
>RMB on the urgent path, (b) the receive length in smc_rx_recvmsg(), and (c) the
>send length in smc_tx_sendmsg() on the SMC-D DMB-merge path. A malicious or
>buggy peer can forge a cursor so each runs past the relevant buffer: an
>out-of-bounds read of adjacent kernel memory (disclosed to the peer) on the
>receive/urgent side, and, on the send side, an out-of-bounds write whose
>length the peer controls and whose overflowing bytes are the local sender's
>own outbound data.
>
>This series bounds each length where it is consumed. The clamp is synchronous
>and race-free against the tasklet that advances the cursor, so it is the minimal
>fix for stable. A separate net-next series adds the wire-boundary validation and
>connection abort that Dust Li suggested; those do not replace these clamps.
>
>The clamp is not subsumed by validating cursors at the input boundary. A peer
>that only increments prod.wrap with count == 0 hits the differing-wrap branch of
>smc_curs_diff(), which returns (len - 0) + 0 == len every CDC, so bytes_to_rcv
>(and sndbuf_space on the send side) accumulates past the buffer while every
>per-cursor bound sees count == 0 and accepts the message. The overflow lives in
>the accumulator, not the cursor; only the consumer-side clamp bounds it. And
>because a queued abort runs asynchronously (queue_work -> smc_conn_kill) while
>smc_rx_recvmsg() reads the accumulator under lock_sock, only the synchronous
>clamp closes that window. So the clamp goes to stable; the abort is net-next.
>
>The nearby readable >= rmb_desc->len / len > sndbuf_desc->len tests only feed
>statistics counters (SMC_STAT_RMB_RX_FULL / SMC_STAT_RMB_TX_SIZE_SMALL) on an
>earlier, separate read; they do not bound the copy.
>
>A/B (in-kernel KASAN replaying the sink arithmetic over a real rmb_desc->len /
>sndbuf_desc->len slab; kasan.fault=report kasan_multi_shot, 2026-07-05):
>  - urgent index (1/3):  count = len+1        -> slab-out-of-bounds Read;  clamped -> clean
>  - recv length  (2/3):  bytes_to_rcv = 5*len via wrap++/count=0 -> OOB Read; clamped -> clean
>  - send length  (3/3):  sndbuf_space inflated -> slab-out-of-bounds Write; clamped -> clean
>  - signed overflow:     readable = -1 -> v1 ">len" misses -> OOB; "<0 || >len" -> clean
>  - concurrent TOCTOU race: a producer-side clamp is racy (OOB in a racing consumer
>    kthread on another CPU); the consumer-side clamp is race-free (0 hits / 5,000,000 reads).
>  - every in-bounds / honest-peer arm: clean.
>
>Changes since v3:
>  - split into this stable-bound clamp series and a separate net-next
>    validate/abort series, per Dust Li's review;
>  - tightened the commit messages; noted that the nearby SMC_STAT_* tests are not
>    bounds; no functional change to the three clamps.
>  v3: https://lore.kernel.org/all/20260614-b4-disp-edd64be9-v3-0-551fa514257e@proton.me/

Hi Bryam,

Are you planning to land these clamps first, and then follow up with a
separate validate/abort series?

Looking at your earlier A/B test, it simulates this logic in userspace to
demonstrate the bug, but it doesn't actually trigger the bug in our
current kernel. If that's the case, the security risk here doesn't seem
high to me, since SMC is only meant to be deployed in trusted environments.

On the other hand, once this is actually triggered, it means the data
we've been handing to userspace is already wrong, which is already a
serious problem, and the connection should be terminated. So I don't
really see much value in merging the bound-clamp patches first.

So, I'd expected to see the real bug and validation/abort patch.

Best regards,
Dust