Re: [PATCH net v2] net: qualcomm: rmnet: require real-netns admin for bridging

[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: 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).

- [High] The patch closes the cross-netns real-port privilege gap only in
  the bridge ndo callbacks.
- [Medium] The commit message says it will 'Require CAP_NET_ADMIN in the
  real device's namespace before ... removing a bridge'.

--

net: qualcomm: rmnet: require real-netns admin for bridging

An rmnet device keeps its link to the real device after it is moved to
another network namespace. This patch adds rtnl_dev_link_net_capable()
checks to rmnet_add_bridge() and rmnet_del_bridge(). With the checks,
changing the bridge state of the real port needs CAP_NET_ADMIN in the
real device's namespace.

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
> index bed6f63facf25..62d7acb13e0c0 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
> @@ -441,6 +441,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(slave_dev),
> +	 * but bridge mode below controls real_dev, which may live in another
> +	 * netns.
> +	 */
> +	if (!rtnl_dev_link_net_capable(slave_dev, dev_net(real_dev)))
> +		return -EPERM;
> +

[Severity: High]
The bridge ops are not the only rtnl entry point that modifies the shared
real port. Should rmnet_changelink() get the same check?

rmnet_changelink() looks up the port shared by every rmnet endpoint on
real_dev and modifies it. The only capability check is the one rtnetlink
does in dev_net(dev):

drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c:rmnet_changelink() {
    ...
	port = rmnet_get_port_rtnl(real_dev);

	if (data[IFLA_RMNET_MUX_ID]) {
		...
			hlist_del_init_rcu(&ep->hlnode);
			hlist_add_head_rcu(&ep->hlnode,
					   &port->muxed_ep[mux_id]);
		...
	}

	if (data[IFLA_RMNET_FLAGS]) {
		...
		port->data_format &= ~flags->mask;
		port->data_format |= flags->flags & flags->mask;

		if (rmnet_vnd_update_dev_mtu(port, real_dev)) {
    ...
}

Suppose an rmnet device has been moved into netns B while real_dev stays
in netns A. A user with CAP_NET_ADMIN only in B can then send
RTM_NEWLINK with IFLA_RMNET_FLAGS. That rewrites port->data_format,
which controls deaggregation, MAP checksum offload and MAPv5 handling
for every rmnet endpoint on real_dev in A.

The same user can also move the endpoint between port->muxed_ep[]
buckets by changing IFLA_RMNET_MUX_ID.

rmnet_vnd_update_dev_mtu() walks every endpoint on the port and calls
rmnet_vnd_change_mtu(ep->egress_dev, real_dev->mtu - headroom). So
setting a flag that grows the headroom, such as
RMNET_FLAGS_EGRESS_MAP_CKSUMV4, also shrinks the MTU of rmnet devices
in netns A.

macvlan_changelink() already guards shared lower-port settings with:

	!rtnl_dev_link_net_capable(dev, dev_net(vlan->lowerdev))

Would a similar guard in rmnet_changelink(), at least for
IFLA_RMNET_FLAGS and IFLA_RMNET_MUX_ID, be needed for this fix to be
complete? The other option would be to mark rmnet devices
netns_immutable.

>  	port = rmnet_get_port_rtnl(real_dev);
>  
>  	/* If there is more than one rmnet dev attached, its probably being
> @@ -489,7 +496,18 @@ int rmnet_add_bridge(struct net_device *rmnet_dev,
>  int rmnet_del_bridge(struct net_device *rmnet_dev,
>  		     struct net_device *slave_dev)
>  {
> -	struct rmnet_port *port = rmnet_get_port_rtnl(slave_dev);
> +	struct rmnet_priv *priv = netdev_priv(rmnet_dev);
> +	struct net_device *real_dev = priv->real_dev;
> +	struct rmnet_port *port;
> +
> +	/* The rtnl path only checks CAP_NET_ADMIN against dev_net(slave_dev),
> +	 * but bridge teardown also updates real_dev, which may live in another
> +	 * netns.
> +	 */
> +	if (!rtnl_dev_link_net_capable(slave_dev, dev_net(real_dev)))
> +		return -EPERM;

[Severity: Medium]
The commit message says:

    Require CAP_NET_ADMIN in the real device's namespace before adding or
    removing a bridge.

This check gates only the ndo_del_slave path. Two other teardown paths
reach rmnet_unregister_bridge() and make the same real port change:

	real_port->bridge_ep = NULL;
	real_port->rmnet_mode = RMNET_EPMODE_VND;

The first is RTM_DELLINK on the rmnet device. rtnl_delete_link() calls
ops->dellink directly:

rtnl_delete_link()->rmnet_dellink() {
    ...
	bridge_dev = real_port->bridge_ep;
	if (bridge_dev) {
		bridge_port = rmnet_get_port_rtnl(bridge_dev);
		rmnet_unregister_bridge(bridge_port);
	}
    ...
}

The second is NETDEV_UNREGISTER on the slave, which fires when a
virtual slave is deleted or moved to another netns:

rmnet_config_notify_cb()->rmnet_force_unassociate_device() {
    ...
	} else {
		rmnet_unregister_bridge(port);
	}
}

Neither path has a real-netns capability check. Both are void or
notifier paths, so they cannot fail the same way.

A user who is admin only in the rmnet/slave netns would now get -EPERM
from "ip link set slave nomaster" or from a master change. In that case
do_set_master() returns after ndo_del_slave fails, before it attaches the
new master. The same user can still remove the bridge without
real-netns privilege by deleting the rmnet device, or the slave if it is
virtual.

Does the del-side check give any real protection here? The add-side
check already blocks unprivileged bridge creation. Would it make sense
to drop or rethink this check rather than block the clean unenslave
path?

> +
> +	port = rmnet_get_port_rtnl(slave_dev);
>  
>  	rmnet_unregister_bridge(port);
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261004203149.3042491-2-Jeremy.Jean%40oss.cyber.gouv.fr