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

[Bryam Vargas <hexlabsecurity@proton.me>]

On Tue, 7 Jul 2026 17:29:04 +0800, Dust Li wrote:
> Are you planning to land these clamps first, and then follow up with a
> separate validate/abort series?

Yes -- clamp series to net (Cc: stable), then the wire-boundary validate/abort to
net-next, which is the split from your v3 review. If you'd rather have the
validate/abort as the primary fix, or both in one series, say so and I'll
restructure it.

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

Right -- the earlier one replayed the smc_curs_diff/copy arithmetic over a kmalloc
buffer. I built the end-to-end version: two AF_SMC sockets over the SMC-D loopback
(dibs), CONFIG_SMC=m with KASAN, receive path unmodified. Only the sender's on-wire
producer cursor is forged, modelling what a misbehaving peer sends:

  cdc.prod.wrap = curs.wrap;
  cdc.prod.count = curs.count;
+ if (forge) {                 /* peer just bumps the wrap, count stays 0 */
+     static u16 w;
+     cdc.prod.wrap = ++w;
+     cdc.prod.count = 0;
+ }

The client sends six 1-byte messages, the server recvs into a 2 MB buffer.
rmb_desc->len = 65504; the three arms on 7.2-rc1:

  honest (no forge)            recv 6        clean
  forged, patch 2/3 clamp on   recv 65504    clean   (== rmb_desc->len)
  forged, no clamp             recv 393024   KASAN

In the last arm bytes_to_rcv reaches 6*len, so smc_rx_recvmsg()'s second wrap-around
chunk (copylen - first_chunk = 393024 - 65504) is read from ring offset 0, past the
RMB page:

  BUG: KASAN: slab-use-after-free in _copy_to_iter
  Read of size 327520 ... smc_rx_recvmsg <- smc_recvmsg <- __sys_recvfrom

(use-after-free rather than out-of-bounds only because the over-read lands in a freed
adjacent slab.) Happy to send the driver.

> the security risk here doesn't seem high to me, since SMC is only meant to
> be deployed in trusted environments.

Agreed it's low urgency there. The reason I'd still keep the bound in stable: it's a
peer-driven out-of-bounds read of kernel memory that a buggy, not only malicious,
peer can hit, and the clamp never resets an honest connection. The stable tag is
your call.

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

I'm not arguing against the abort -- a bad CDC means the connection can't be trusted
and should go down, and that's the net-next work. Two points on it.

The predicate has to test the accumulator, not the cursor. Every forged CDC here
carries count == 0, which is in [0, rmb_desc->len), so it passes any per-cursor
input check, including patch 1/3; only bytes_to_rcv goes out of range. A
cursor-boundary abort wouldn't catch this vector.

And placement: if the abort is queued (queue_work -> smc_conn_kill) after the
atomic_add, a recvmsg() under lock_sock can still read the inflated accumulator in
the window before teardown runs. A synchronous check that bails before the
atomic_add avoids that, and so does the consumer clamp.

If you'd prefer a single accumulator-abort in place of the -stable clamp, I'll
respin it that way and run the same A/B.

Bryam