Re: [PATCH net] net: devmem: check uid and netns in net_devmem_get_binding()

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] The patch narrows who may transmit from a TX dmabuf binding.

--

net: devmem: check uid and netns in net_devmem_get_binding()

This commit stores the network namespace and sk_uid of the netlink socket
that created a dmabuf binding. net_devmem_get_binding() now rejects TX
sockets whose netns or sk_uid does not match. On a mismatch it returns
-EINVAL, the same error as a non-existent binding ID.

> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index f4d60654ce7fd..9a773b1ce92de 100644
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c

[ ... ]

> @@ -220,6 +221,8 @@ net_devmem_bind_dmabuf(struct net_device *dev, void *vdev,
>  
>  	binding->dev = dev;
>  	binding->vdev = vdev;
> +	write_pnet(&binding->net, sock_net(nl_sk));
> +	binding->uid = sk_uid(nl_sk);
>  	binding->niov_shift = niov_shift;
>  	xa_init_flags(&binding->bound_rxqs, XA_FLAGS_ALLOC);
>  
> @@ -390,7 +393,9 @@ struct net_devmem_dmabuf_binding *net_devmem_get_binding(struct sock *sk,
>  	int err = 0;
>  
>  	binding = net_devmem_lookup_dmabuf(dmabuf_id);
> -	if (!binding || !binding->tx_vec) {
> +	if (!binding || !binding->tx_vec ||
> +	    !net_eq(sock_net(sk), read_pnet(&binding->net)) ||
> +	    !uid_eq(sk_uid(sk), binding->uid)) {
>  		err = -EINVAL;
>  		goto out_err;
>  	}

[Severity: Medium]
Could this break existing devmem TX users whose TCP socket and netlink
socket have different owners?

binding->uid comes from sk_uid(nl_sk), and the new check compares it with
sk_uid(sk) of the sending socket. sk_uid is the socket's owner. It is set
when the socket is created, in sock_init_data_uid(), or changed later by
sockfs_setattr(). It is not the caller's current credentials.

sock_graft() does not update sk_uid. An accepted TCP socket therefore keeps
the listener's sk_uid through sk_clone.

In these setups, every devmem sendmsg through
tcp_sendmsg_locked()->net_devmem_get_binding() would now fail with -EINVAL:

  - A listener created by root, or through systemd socket activation, where
    the service opens the netdev netlink socket after dropping privileges.
    binding->uid is then the service uid, while the accepted socket's
    sk_uid is 0.
  - A privileged helper that runs bind-tx and hands the binding IDs to
    workers running as other uids.
  - Sockets passed between uids with SCM_RIGHTS.

The mismatch returns the same -EINVAL as a stale ID. How would userspace
tell these cases apart?

Should Documentation/networking/devmem.rst be updated for the new
requirement? It still describes the ID as:

    The netlink API returns a dmabuf_id: a unique ID that refers to this
    dmabuf that has been bound.

It also lists only SO_ZEROCOPY / MSG_ZEROCOPY, plus an optional
SO_BINDTODEVICE, as TX socket requirements.

The comment above netdev_nl_bind_tx_doit() says:

net/core/netdev-genl.c:
 * 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

That is not quite the same rule as "same sk_uid and same netns".

Because of the Fixes: tag, this restriction would also be backported to
stable. Should the commit message describe the compatibility impact?

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