Re: [PATCH net v4] net: qualcomm: rmnet: require CAP_NET_ADMIN in the real device netns for config ops

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 1

- [High] Incomplete cross-netns authorization: the ethtool coalesce
  setter rmnet_set_coalesce()…
- [Low] The two new -EPERM returns that have an extack available carry no
  extended ACK message.

--

net: qualcomm: rmnet: require CAP_NET_ADMIN in the real device netns for
config ops

This adds an rtnl_dev_link_net_capable() gate to rmnet_changelink(),
rmnet_add_bridge() and rmnet_del_bridge() so that a caller privileged only
in the rmnet device's netns cannot rewrite the rmnet port state attached to
a real device living in another netns.  Two observations below on the set
of paths covered and on the error reporting.

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
> index bed6f63facf25..5d0a4a428e97a 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
> @@ -312,6 +312,13 @@ static int rmnet_changelink(struct net_device *dev, struct nlattr *tb[],
>  	if (!rmnet_is_real_dev_registered(real_dev))
>  		return -ENODEV;
>  
> +	/* The rtnl path only checks CAP_NET_ADMIN against dev_net(dev),
> +	 * but the port state mutated below is attached to real_dev, which
> +	 * may live in a different netns.
> +	 */
> +	if (!rtnl_dev_link_net_capable(dev, dev_net(real_dev)))
> +		return -EPERM;
> +

[Severity: High]
Is the ethtool coalesce setter intentionally left out of this series?

rmnet_set_coalesce() in rmnet_vnd.c reaches the same shared port through
the rmnet device and rewrites it, with no netns check:

drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c:rmnet_set_coalesce() {
	struct rmnet_priv *priv = netdev_priv(dev);
	struct rmnet_port *port;

	port = rmnet_get_port_rtnl(priv->real_dev);

	if (kernel_coal->tx_aggr_max_frames < 1 || kernel_coal->tx_aggr_max_frames > 64)
		return -EINVAL;

	if (kernel_coal->tx_aggr_max_bytes > 32768)
		return -EINVAL;

	rmnet_map_update_ul_agg_config(port, kernel_coal->tx_aggr_max_bytes,
				       kernel_coal->tx_aggr_max_frames,
				       kernel_coal->tx_aggr_time_usecs);
}

rmnet_map_update_ul_agg_config() then stores into the real device's port:

	spin_lock_bh(&port->agg_lock);
	port->egress_agg_params.bytes = size;
	WRITE_ONCE(port->egress_agg_params.count, count);
	port->egress_agg_params.time_nsec = time * NSEC_PER_USEC;
	spin_unlock_bh(&port->agg_lock);

The authorization on that path is only against the netns the rmnet device
was addressed through: dev_ethtool() checks
ns_capable(net->user_ns, CAP_NET_ADMIN), and ETHTOOL_MSG_COALESCE_SET is
registered with GENL_UNS_ADMIN_PERM.  Neither consults
dev_net(priv->real_dev).

Can that value also stop the uplink traffic of the other netns?
tx_aggr_max_bytes == 0 with tx_aggr_max_frames >= 2 passes the validation
above, and the aggregation path in rmnet_map_tx_aggregate() then computes:

		size = port->egress_agg_params.bytes - skb->len;

		if (size < 0) {
			/* dropped */
			spin_unlock_bh(&port->agg_lock);
			return 0;
		}

which is taken for every nonempty packet on that port, for all endpoints
muxed on the real device, and is turned into -ENOMEM by
rmnet_map_egress_handler().

Would it make sense to add the same
rtnl_dev_link_net_capable(dev, dev_net(priv->real_dev)) gate to
rmnet_set_coalesce() with a Fixes: tag for the coalesce support commit, or
alternatively to note in the changelog why this op is excluded?

>  	port = rmnet_get_port_rtnl(real_dev);
>  
>  	if (data[IFLA_RMNET_MUX_ID]) {
> @@ -441,6 +448,13 @@ int rmnet_add_bridge(struct net_device *rmnet_dev,
>  	struct rmnet_port *port, *slave_port;
>  	int err;
>  
> +	/* The rtnl path only checks CAP_NET_ADMIN against dev_net(dev),
> +	 * but the port state mutated below is attached to real_dev, which
> +	 * may live in a different netns.
> +	 */
> +	if (!rtnl_dev_link_net_capable(rmnet_dev, dev_net(real_dev)))
> +		return -EPERM;
> +

[Severity: Low]
This isn't a bug, but should these two -EPERM returns carry an extack
message?  Both rmnet_changelink() and rmnet_add_bridge() get a
struct netlink_ext_ack *extack and already describe every other failure
that way, for example "MUX ID already exists", "Invalid MTU on real dev",
"more than one rmnet dev attached", "more than one bridge dev attached" and
"slave cannot be another rmnet dev", while the new capability failure
returns a bare -EPERM.

The equivalent gate in macvlan does report it, in both
macvlan_common_newlink() and macvlan_changelink():

drivers/net/macvlan.c:macvlan_changelink() {
	    !rtnl_dev_link_net_capable(dev, dev_net(vlan->lowerdev))) {
		NL_SET_ERR_MSG(extack,
			       "Changing shared macvlan port settings requires CAP_NET_ADMIN in the lower device network namespace");
		return -EPERM;
}

rmnet_del_bridge() has no extack argument, so only the changelink and
add_bridge sites can do this without a signature change.

>  	port = rmnet_get_port_rtnl(real_dev);
>  
>  	/* If there is more than one rmnet dev attached, its probably being

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260901081441.632-1-suruurism%40gmail.com