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-11 10:43:26, Bryam Vargas wrote:
>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.

Hi Bryam,

Thanks for the detailed explanation — I think you're right. To me the
key point here is that no matter how the peer misbehaves, we should
never let it cause a panic or memory corruption on our side.

So I think your current clamp can stay. In the net-next version we
can add the abort logic, and additionally, it would be best to emit a
warning log whenever the clamp detects an anomaly here.

Best regards, Dust