Re: [PATCH net] net/smc: validate peer CDC cursor against RMBE size before accepting it
Bryam Vargas <[email protected]>
| Newsgroups | org.kernel.vger.linux-rdma,org.kernel.vger.linux-kernel,org.kernel.vger.linux-s390,org.kernel.vger.netdev |
|---|---|
| Message-ID | <[email protected]> |
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