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
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.