Re: [PATCH net-next v1] net: devmem: document that bind-tx is unprivileged by design

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 2 · 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] NETDEV_CMD_BIND_TX is unprivileged (net/core/netdev-genl-
  gen.c:240-245: .flags = GENL_CMD_CAP_DO), and this patch cements that…
- [Medium] The comment added above netdev_nl_bind_tx_doit()
  (net/core/netdev-genl.c:1183-1188) and the mirrored note in…
- [Medium] bind-tx is reachable by any unprivileged user
  (net/core/netdev-genl-gen.c:244: .flags = GENL_CMD_CAP_DO) with no…

--

net: devmem: document that bind-tx is unprivileged by design

This adds a comment above netdev_nl_bind_tx_doit() and a matching note in
Documentation/netlink/specs/netdev.yaml stating that NETDEV_CMD_BIND_TX
carries no GENL_ADMIN_PERM / GENL_UNS_ADMIN_PERM on purpose, on the grounds
that bind-tx only DMA-maps the caller's dmabuf.

A few questions about the rationale being recorded here.

> diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml
> index e13461cf3fc33..4303593e5003c 100644
> --- a/Documentation/netlink/specs/netdev.yaml
> +++ b/Documentation/netlink/specs/netdev.yaml
> @@ -852,6 +852,8 @@ operations:
>        name: bind-tx
>        doc: Bind dmabuf to netdev for TX
>        attribute-set: dmabuf
> +      # Intentionally unprivileged (no admin-perm / uns-admin-perm); see
> +      # comment above netdev_nl_bind_tx_doit().
>        do:
>          request:
>            attributes:
> diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> index fa9edfdb32c29..690c27f63d260 100644
> --- a/net/core/netdev-genl.c
> +++ b/net/core/netdev-genl.c
> @@ -1180,6 +1180,12 @@ netdev_find_netmem_tx_dev(struct net_device *dev)
>  	return NULL;
>  }
>  
> +/* Note: NETDEV_CMD_BIND_TX is intentionally unprivileged (no
> + * GENL_ADMIN_PERM / GENL_UNS_ADMIN_PERM). Unlike bind-rx, which configures
> + * shared NIC RX queues, bind-tx only DMA-maps the caller's dmabuf so they can
> + * transmit from it on their own sockets without affecting other traffic or
> + * device state.
> + */

[Severity: High]
Is the "on their own sockets" part of this rationale actually enforced
anywhere?

net_devmem_bind_dmabuf() in net/core/devmem.c allocates the id out of a
single global xarray and only records the owning netlink socket for cleanup:

	err = xa_alloc_cyclic(&net_devmem_dmabuf_bindings, &binding->id,
			      binding, xa_limit_32b, &id_alloc_next,
			      GFP_KERNEL);
	...
	list_add(&binding->list, &priv->bindings);

priv->bindings is then only walked by netdev_nl_sock_priv_destroy().

On the TX side, net_devmem_get_binding() resolves whatever u32 the sender
passed and checks only that a tx_vec exists and that the route egresses the
bound device:

	binding = net_devmem_lookup_dmabuf(dmabuf_id);
	if (!binding || !binding->tx_vec) {
		err = -EINVAL;
		goto out_err;
	}
	...
	dst_dev = dst_dev_rcu(dst);
	if (unlikely(!dst_dev) ||
	    unlikely(dst_dev != READ_ONCE(binding->dev) &&
		     dst_dev != READ_ONCE(binding->vdev))) {
		err = -ENODEV;

and net_devmem_lookup_dmabuf() is a plain xa_load() with no credential,
netns or creator comparison.

So can an unrelated unprivileged process reach another user's binding like
this?

  process A: NETDEV_CMD_BIND_TX -> gets id N, keeps the netlink socket open
  process B: sendmsg(MSG_ZEROCOPY) with SCM_DEVMEM_DMABUF carrying id N

__sock_cmsg_send() in net/core/sock.c takes the bare id:

	case SCM_DEVMEM_DMABUF:
		if (cmsg->cmsg_len != CMSG_LEN(sizeof(u32)))
			return -EINVAL;
		sockc->dmabuf_id = *(u32 *)CMSG_DATA(cmsg);

tcp_sendmsg_locked() then calls net_devmem_get_binding(sk, sockc.dmabuf_id)
and zerocopy_fill_skb_from_devmem() builds frags from the resulting niovs at
attacker-chosen offsets. If B's route egresses the same netdev, does this
let B transmit the contents of A's dmabuf to a peer of B's choosing?

Ids are allocated cyclically starting low, and the return codes differ
(-EINVAL for an unknown or non-TX id, -ENODEV for the wrong device), so is
guessing a live id cheap? Should the TX path check the binding against its
creating netdev_nl_sock before this is documented as safe, or should the
command grow a permission flag instead?

[Severity: Medium]
Separately from the enforcement question above, is the wording here wider
than what the code does?

Two parts look overstated:

"so they can transmit from it on their own sockets" - nothing in
net_devmem_get_binding() ties a binding to the socket, task, uid, netns or
cgroup that created it; the u32 id from the global xarray is the only
credential.

"without affecting other traffic or device state" -
netdev_find_netmem_tx_dev() can resolve the bind target to a different,
shared physical NIC via an rx queue lease:

		phys_dev = lease_rxq->dev;
		if (netif_device_present(phys_dev) &&
		    phys_dev->netmem_tx == NETMEM_TX_DMA)
			return phys_dev;

and net_devmem_bind_dmabuf() then runs dma_buf_attach() and
dma_buf_map_attachment_unlocked() against that physical device's dma_dev.

Would it be more accurate to state the narrow property that bind-tx does
not reconfigure RX queues or steering, rather than claiming no effect on
other traffic or device state? The same wording is mirrored into the uAPI
spec in Documentation/netlink/specs/netdev.yaml, so it becomes the answer
future readers get when they ask why this command has no permission flag.

[Severity: Medium]
Does anything bound how many times an unprivileged caller can do this, or
account the memory it consumes?

net_devmem_bind_dmabuf() can be called repeatedly on the same dmabuf FD, and
each call allocates with plain GFP_KERNEL / kvmalloc, so none of it is
charged to the caller's memcg:

		binding->tx_vec = kvmalloc_objs(struct net_iov *,
						dmabuf->size / PAGE_SIZE);
	...
		owner->area.niovs = kvmalloc_objs(*owner->area.niovs,
						  owner->area.num_niovs);

and each call installs another mapping on the shared device:

	binding->attachment = dma_buf_attach(binding->dmabuf, dma_dev);
	...
	binding->sgt = dma_buf_map_attachment_unlocked(binding->attachment,
						       direction);

There is no duplicate-binding check, no count limit on entries added to
priv->bindings, no __GFP_ACCOUNT, and no RLIMIT_MEMLOCK accounting for the
long-lived mapping. With the rx-lease path in netdev_find_netmem_tx_dev()
these mappings land on the physical NIC shared with other users.

Given that, is "without affecting other traffic or device state" the right
thing to document, or should a per-socket or per-user cap and memcg
accounting come first?

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