Re: [PATCH net v3] 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 3 potential
issue(s) to consider:

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

- [Medium] Wrong reference netns in the new capability gate on the
  ndo_del_slave path.
- [Low] The two comments added above the new gates in rmnet_add_bridge()
  (lines 452-456) and rmnet_del_bridge() (lines 512-516) were copied…
- [Low] The new capability rejections return -EPERM silently.

--

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

This adds rtnl_dev_link_net_capable() gates to rmnet_changelink(),
rmnet_add_bridge() and rmnet_del_bridge() so that a caller privileged only
in the rmnet device's netns can no longer rewrite the rmnet port state
attached to a real device living in another netns.  rmnet_del_bridge() also
grows local priv/real_dev lookups for the new check.

A few questions below.

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
> index bed6f63facf2..6feafc55a7e9 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
> @@ -312,6 +312,14 @@ 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: Low]
This isn't a bug, but should the new rejection report through extack?

Both rmnet_changelink() and rmnet_add_bridge() take a
struct netlink_ext_ack *extack, and every other error return in those two
functions fills it in:

	NL_SET_ERR_MSG_MOD(extack, "MUX ID already exists");
	NL_SET_ERR_MSG_MOD(extack, "Invalid MTU on real dev");
	NL_SET_ERR_MSG_MOD(extack, "more than one rmnet dev attached");
	NL_SET_ERR_MSG_MOD(extack, "more than one bridge dev attached");
	NL_SET_ERR_MSG_MOD(extack, "slave cannot be another rmnet dev");

The closest peer users of the same gate do report the reason, e.g. in
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;

Since the generic rtnetlink capability check also returns -EPERM for
RTM_NEWLINK/RTM_SETLINK, can user space tell the two denials apart as
written?

>  	port = rmnet_get_port_rtnl(real_dev);
>  
>  	if (data[IFLA_RMNET_MUX_ID]) {

[ ... ]

> @@ -441,6 +449,14 @@ 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.
> +	 */

[Severity: Low]
This isn't a bug, but the comment appears to have been copied from the
rmnet_changelink() hunk without adjusting it.

rmnet_add_bridge() and rmnet_del_bridge() have no dev parameter or local:

	int rmnet_add_bridge(struct net_device *rmnet_dev,
			     struct net_device *slave_dev,
			     struct netlink_ext_ack *extack)

	int rmnet_del_bridge(struct net_device *rmnet_dev,
			     struct net_device *slave_dev)

The rtnl entry point for these two is RTM_SETLINK IFLA_MASTER via
do_set_master() and ndo_add_slave/ndo_del_slave, where the caller was
checked against dev_net(slave_dev) rather than a changelink on dev.  Could
both comments name slave_dev instead of the non-existent dev?

Also, the three added blocks use the form with '/*' alone on the first
line, while the surrounding comments in this file put text on the first
line, e.g. just below in the same function:

	/* If there is more than one rmnet dev attached, its probably being
	 * used for muxing. Skip the briding in that case
	 */

> +	if (!rtnl_dev_link_net_capable(rmnet_dev, dev_net(real_dev)))
> +		return -EPERM;
> +
>  	port = rmnet_get_port_rtnl(real_dev);
>  
>  	/* If there is more than one rmnet dev attached, its probably being

[ ... ]

> @@ -489,7 +505,19 @@ 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(dev),
> +	 * but rmnet_unregister_bridge() below clears the bridge state of
> +	 * the real device's port, which may live in a different netns.
> +	 */
> +	if (!rtnl_dev_link_net_capable(rmnet_dev, dev_net(real_dev)))
> +		return -EPERM;

[Severity: Medium]
Is rmnet_dev the right reference device for the gate on the del path?

rtnl_dev_link_net_capable() short-circuits when the two namespaces are
equal:

net/core/rtnetlink.c:rtnl_dev_link_net_capable() {
	return net_eq(link_net, dev_net(dev)) ||
	       ns_capable(link_net->user_ns, CAP_NET_ADMIN);
}

On ndo_del_slave the netns the rtnl layer actually verified CAP_NET_ADMIN
against is dev_net(slave_dev), and rmnet_dev is simply whatever master is
already linked:

net/core/rtnetlink.c:do_set_master() {
	struct net_device *upper_dev = netdev_master_upper_dev_get(dev);
	...
		if (ops->ndo_del_slave) {
			err = ops->ndo_del_slave(upper_dev, dev);
	...
}

There is no netns-equality requirement on that master link, and nothing
seems to forbid moving the rmnet vnd device out of the slave's netns:
rmnet_vnd_setup() does not set dev->netns_immutable,
__dev_change_net_namespace() only refuses netns_immutable devices, and
rmnet_config_notify_cb() reacts to NETDEV_UNREGISTER only when
rmnet_is_real_dev_registered(dev) is true, which is false for the vnd
device, so moving rmnet_dev does not tear the bridge down.

If rmnet_dev ends up in real_dev's netns A while slave_dev stays in netns
B, then net_eq(dev_net(real_dev), dev_net(rmnet_dev)) is true, ns_capable()
is never evaluated, and a caller holding CAP_NET_ADMIN only in B can run
"ip link set <slave> nomaster" and reach:

drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c:rmnet_unregister_bridge() {
		real_port = rmnet_get_port_rtnl(real_dev);
		real_port->bridge_ep = NULL;
		real_port->rmnet_mode = RMNET_EPMODE_VND;
}

which is the cross-netns port mutation the commit message describes.
Reaching that state does need a prior CAP_NET_ADMIN-in-A action to move
rmnet_dev, so the window is narrow.

Would using slave_dev as the reference device on both bridge paths close
this?  On the add path the two are equivalent, since do_set_master()
resolves the new master with __dev_get_by_index(dev_net(slave_dev),
ifindex), so passing slave_dev there too would make the invariant
explicit.

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

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