Re: [PATCH net v1] net: devmem: prevent mixing fragments from different bindings

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 2 (1) · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] This patch makes a valid devmem MSG_ZEROCOPY sendmsg() fail with
  -EFAULT.
- [High] The commit says it prevents mixing fragments from different
  devmem bindings in one skb, but it only guards the…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it undermines
  the validation the patch hardens.

--

net: devmem: prevent mixing fragments from different bindings

This patch adds net_devmem_skb_binding(), which returns the devmem binding
of an unreadable skb's first frag. validate_xmit_unreadable_skb() now uses
it, and zerocopy_fill_skb_from_devmem() now refuses to append frags from a
different binding to an skb that already has frags. The aim is to stop
consecutive sendmsg() calls with different dmabuf bindings from sharing
one skb and getting past the per-device and unbind checks at xmit time.

> diff --git a/net/core/datagram.c b/net/core/datagram.c
> index 173b5d97bd409..ed8f1045f3cca 100644
> --- a/net/core/datagram.c
> +++ b/net/core/datagram.c
> @@ -712,7 +712,7 @@ zerocopy_fill_skb_from_devmem(struct sk_buff *skb, struct iov_iter *from,
>  	size_t virt_addr, size, off;
>  	struct net_iov *niov;
>  
> -	if (i && skb_frags_readable(skb))
> +	if (i && net_devmem_skb_binding(skb) != binding)
>  		return -EFAULT;

[Severity: High]
Can this make a valid devmem MSG_ZEROCOPY sendmsg() fail with -EFAULT?

Segment selection in tcp_sendmsg_locked() still compares only readability,
not binding identity:

net/ipv4/tcp.c:tcp_sendmsg_locked() {
    ...
		if (copy <= 0 || !tcp_skb_can_collapse_to(skb) ||
		    unlikely(skb_frags_readable(skb) != !binding)) {
    ...
}

Say the unsent tail skb holds frags from binding A, and the next sendmsg()
passes binding B through SCM_DEVMEM_DMABUF. The condition above is false,
so TCP appends to the tail skb.

On back-to-back calls, msg_zerocopy_realloc() extends and returns the same
uarg, so skb_zerocopy_iter_stream() does not return -EEXIST. We then reach
the new check with nr_frags > 0 and net_devmem_skb_binding(skb) == A != B.

The -EFAULT is not one of the errors that start a new segment:

net/ipv4/tcp.c:tcp_sendmsg_locked() {
    ...
			err = skb_zerocopy_iter_stream(sk, skb, msg, copy, uarg,
						       binding);
			if (err == -EMSGSIZE || err == -EEXIST) {
				tcp_mark_push(tp, skb);
				goto new_segment;
			}
			if (err < 0)
				goto do_error;
    ...
}

With copied == 0, userspace gets -EFAULT. Retries fail the same way until
the tail skb leaves the write queue. That can take a while if the socket
is corked, MSG_MORE is used, Nagle or TSO defers, or cwnd is the limit.

The old "i && skb_frags_readable(skb)" check could not be reached from
TCP, because the tcp_sendmsg_locked() condition above already filtered
that case. The new check can be reached.

net_devmem_get_binding() accepts any binding whose dev (or vdev) matches
the socket's dst device. So using two dmabufs bound to the same NIC on one
socket is legitimate, and it worked before this patch.

Would it be better to start a new segment when the binding changes? One
option is to add net_devmem_skb_binding(skb) != binding to the
tcp_sendmsg_locked() condition. Another is to return -EEXIST or -EMSGSIZE
here.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index e76762e29360e..ad2b587dfee27 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4054,8 +4054,7 @@ static struct sk_buff *sk_validate_xmit_skb(struct sk_buff *skb,
>  static struct sk_buff *validate_xmit_unreadable_skb(struct sk_buff *skb,
>  						    struct net_device *dev)
>  {
> -	struct skb_shared_info *shinfo;
> -	struct net_iov *niov;
> +	struct net_devmem_dmabuf_binding *binding;
>  
>  	if (likely(skb_frags_readable(skb) ||
>  		   dev->netmem_tx == NETMEM_TX_NO_DMA))

[Severity: High]
This issue was not introduced by this patch, but does this early return
let MTU probe skbs skip all of the devmem checks?

tcp_mtu_probe() allocates the probe skb as a readable skb:

net/ipv4/tcp_output.c:tcp_mtu_probe() {
    ...
	nskb = tcp_stream_alloc_skb(sk, GFP_ATOMIC, false);
    ...
}

tcp_clone_payload() then copies the devmem net_iov frags into nskb with
skb_frag_page_copy() and skb_frag_ref(). It never sets to->unreadable; it
only propagates SKBFL_SHARED_FRAG.

So skb_frags_readable(nskb) is true, and validate_xmit_unreadable_skb()
returns here, before the NETMEM_TX_NONE, binding->dev and unbind checks.

The mislabelled skb also looks readable to any code that reads the
payload. With an AF_PACKET tap on the egress device, for example,
tpacket_rcv() calls skb_copy_bits(). That passes its !skb_frags_readable()
guard and then kmaps skb_frag_page(), which is NULL for a net_iov.

packet_rcv() recvmsg, BPF socket filters and skb_checksum_help() appear to
be exposed the same way.

A separate question: skb_frag_page() is NULL for every net_iov, so can the
merge test in tcp_clone_payload() join frags from different niovs?

net/ipv4/tcp_output.c:tcp_clone_payload() {
    ...
			if (lastfrag &&
			    skb_frag_page(fragfrom) == skb_frag_page(lastfrag) &&
			    skb_frag_off(fragfrom) == skb_frag_off(lastfrag) +
						      skb_frag_size(lastfrag)) {
				skb_frag_size_add(lastfrag, todo);
    ...
}

net_devmem_get_niov_at() sets off = virt_addr % PAGE_SIZE. Take an iov at
dmabuf offset 0 with length 1000, followed by an iov at offset
8192 + 1000. That gives niov0 {off 0, len 1000} and niov2 {off 1000}.

Those two frags would be merged into one niov0 frag, and the wrong dmabuf
bytes would go on the wire.

> @@ -4064,14 +4063,9 @@ static struct sk_buff *validate_xmit_unreadable_skb(struct sk_buff *skb,
>  	if (dev->netmem_tx == NETMEM_TX_NONE)
>  		goto out_free;
>  
> -	shinfo = skb_shinfo(skb);
> -
> -	if (shinfo->nr_frags > 0) {
> -		niov = netmem_to_net_iov(skb_frag_netmem(&shinfo->frags[0]));
> -		if (net_is_devmem_iov(niov) &&
> -		    READ_ONCE(net_devmem_iov_binding(niov)->dev) != dev)
> -			goto out_free;
> -	}
> +	binding = net_devmem_skb_binding(skb);
> +	if (binding && READ_ONCE(binding->dev) != dev)
> +		goto out_free;

[Severity: High]
The commit message says this prevents mixing fragments from different
bindings. Is the tcp_sendmsg_locked() append path the only place where
that mixing can happen?

Adjacent write-queue skbs with different bindings can still be created
after this patch. This happens when the binding A skb reached size_goal,
was sent with MSG_EOR, or msg_zerocopy_realloc() allocated a new uarg
(-EEXIST).

With MTU probing enabled and cwnd >= 11, tcp_mtu_probe() checks:

net/ipv4/tcp_output.c:tcp_can_coalesce_send_queue_head() {
    ...
		if (tcp_has_tx_tstamp(skb) || !tcp_skb_can_collapse(skb, next))
			return false;
    ...
}

tcp_skb_can_collapse() compares readability but not the binding:

include/net/tcp.h:tcp_skb_can_collapse() {
	return likely(tcp_skb_can_collapse_to(to) &&
		      mptcp_skb_can_collapse(to, from) &&
		      !skb_cmp_decrypted(to, from) &&
		      skb_pure_zcopy_same(to, from) &&
		      skb_frags_readable(to) == skb_frags_readable(from));
}

tcp_clone_payload() then copies the frags of both skbs into one probe
skb. net_devmem_skb_binding() only looks at frags[0]. So even if the probe
skb were correctly marked unreadable (see above), this check would only
validate binding A's dev. Binding B's dev and unbind state would never be
checked.

The other merge sites look covered. tcp_grow_skb() and SACK shifting go
through skb_shift(), which refuses zerocopy skbs. Retransmit collapse is
blocked because tcp_can_collapse() rejects unreadable skbs.

Should tcp_skb_can_collapse() or tcp_can_coalesce_send_queue_head() also
compare net_devmem_skb_binding() for the two skbs?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008034027.1403188-1-almasrymina%40google.com