Re: [PATCH net] net/smc: validate peer CDC cursor against RMBE size before accepting it

[Hidayathulla Khan I <hidayath@linux.ibm.com>]

Hi Bryam,

I have a standalone net-next patch that aborts the connection when
bytes_to_rcv + diff_prod exceeds rmb_desc->len.  The check sits before
the atomic_add(), so the accumulator is never written with an out-of-range
value:

     if (atomic_read(&conn->bytes_to_rcv) + diff_prod >
         conn->rmb_desc->len) {
         smc_cdc_abort_conn(smc, NULL);
         return;
     }
     smp_mb__before_atomic();
     atomic_add(diff_prod, &conn->bytes_to_rcv);

The patch also moves the open-coded abort sequence in
smc_cdc_msg_validate() into a new smc_cdc_abort_conn() helper, which
the overflow check above then reuses.

It is currently in internal review. I will post it to netdev shortly.

Thanks,
Hidayath

On 24/07/26 4:36 am, Bryam Vargas wrote:
> Agreed, and thanks for dropping the competing one.
>
> Patch 1/3 bounds only the cursor count, not the advance: smc_curs_diff()'s
> differing-wrap branch returns (len - old.count) + new.count, so prod(W,0) then
> prod(W+1,len) is diff_prod == 2*len with both counts in range, and
> smc_cdc_msg_recv_action() adds that to bytes_to_rcv unclamped. wrap++/count==0 does the
> same over several CDCs, so a per-cursor bound can't catch it.
>
> The out-of-bounds copy you point at is closed by patch 2/3, though. It bounds the
> derived readable length at the consumer:
>
> 	if (readable < 0 || readable > conn->rmb_desc->len)
> 		readable = conn->rmb_desc->len;
>
> so copylen <= rmb_desc->len and smc_rx_recvmsg()'s second chunk (copylen - first,
> offset 0) stays within the ring no matter how large bytes_to_rcv grew. I ran your
> wrap++/count=0 vector end to end on the real SMC-D path under KASAN: with 1/3 alone it
> trips slab-out-of-bounds in smc_rx_recvmsg (bytes_to_rcv reaches 6*len, read of size
> 5*len); with 2/3 applied recv() is bounded to rmb_desc->len and it is clean.
>
> What 2/3 does not fix is the accounting: bytes_to_rcv is left > rmb_desc->len (and can
> sign-overflow negative over many messages), so the documented
> 0 <= bytes_to_rcv <= rmb_desc->len invariant is still violated at the producer even
> though the copy is now bounded. I have a net-next follow-up that records and aborts on
> that. Your
>
> 	if (smc_curs_diff(size, &old, &temp) > size)
> 		return;
>
> catches the single-CDC 2*len advance cleanly; the wrap++/count==0 form slips it, since
> each CDC advances exactly len (diff == len, not > len) and it is the accumulated
> bytes_to_rcv that overruns. So I'll pair your advance-bound as an early catch with a
> check on bytes_to_rcv after the atomic_add and abort+record there, which covers both.
> The insight is yours either way -- may I add your Suggested-by (or Co-developed-by)?
>
> The v4 series otherwise stands as reviewed; I'll repost it for the netdev queue and
> keep the accounting bound as the net-next follow-up.
>
> Thanks,
> Bryam
>
>