Re: [PATCH net v6] 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: 1 · Medium: 0 · Low: 2

- [High] The new rtnl_dev_link_net_capable() gate in rmnet_del_bridge()
  (rmnet_config.c:519-529) does not close the bug class the commit…
- [Low] The comment added above the new capability gate in
  rmnet_add_bridge()…
- [Low] The v4 changelog states 'use the netdev comment style, per Subash
  Abhinov Kasiviswanathan', but the block comment newly added in…

--

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

This adds capability gates so that rmnet config operations which mutate the
rmnet_port attached to the real device also require CAP_NET_ADMIN in the real
device's network namespace.  rmnet_changelink(), rmnet_add_bridge() and
rmnet_del_bridge() are gated with rtnl_dev_link_net_capable(), and
rmnet_set_coalesce() with ns_capable() on the real device's user namespace.

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
> index bed6f63facf2..8df9e455110d 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c

[ ... ]

> @@ -441,6 +451,19 @@ int rmnet_add_bridge(struct net_device *rmnet_dev,
>  	struct rmnet_port *port, *slave_port;
>  	int err;
>  
> +	/* The rtnl path authorizes the caller against the RTM_SETLINK
> +	 * target, slave_dev, but the port state mutated below is attached
> +	 * to real_dev, which may live in a different netns.  Check the
> +	 * capability against slave_dev so the master's netns, which the
> +	 * caller was never checked against, cannot short-circuit the gate
> +	 * after being moved into the real device's netns.
> +	 */
> +	if (!rtnl_dev_link_net_capable(slave_dev, dev_net(real_dev))) {

[Severity: Low]

Is the second half of this comment accurate for the add path?  The scenario
it describes is the master having been moved into the real device's netns so
that the net_eq() test inside rtnl_dev_link_net_capable() short-circuits.

On the ndo_add_slave path the master is resolved in the slave's netns by
do_set_master() in net/core/rtnetlink.c:

	upper_dev = __dev_get_by_index(dev_net(dev), ifindex);
	if (!upper_dev)
		return -EINVAL;
	ops = upper_dev->netdev_ops;
	if (ops->ndo_add_slave) {
		err = ops->ndo_add_slave(upper_dev, dev, extack);

Here dev is the slave, so dev_net(rmnet_dev) == dev_net(slave_dev) always
holds by the time rmnet_add_bridge() runs, which makes
rtnl_dev_link_net_capable(slave_dev, dev_net(real_dev)) and
rtnl_dev_link_net_capable(rmnet_dev, dev_net(real_dev)) equivalent at this
call site.

The same wording is accurate in rmnet_del_bridge(), where the master comes
from netdev_master_upper_dev_get(dev) with no per-netns lookup.  Would it be
clearer to keep the "moved into the real device's netns" rationale only in
rmnet_del_bridge() and leave a shorter note here?

>  	port = rmnet_get_port_rtnl(real_dev);
>  
>  	/* If there is more than one rmnet dev attached, its probably being
> @@ -489,7 +512,22 @@ 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 authorizes the caller against the RTM_SETLINK
> +	 * target, slave_dev, but rmnet_unregister_bridge() below clears
> +	 * the bridge state of the real device's port, which may live in a
> +	 * different netns.  Check the capability against slave_dev so the
> +	 * master's netns, which the caller was never checked against,
> +	 * cannot short-circuit the gate after being moved into the real
> +	 * device's netns.
> +	 */
> +	if (!rtnl_dev_link_net_capable(slave_dev, dev_net(real_dev)))
> +		return -EPERM;
> +
> +	port = rmnet_get_port_rtnl(slave_dev);
>  
>  	rmnet_unregister_bridge(port);

[Severity: High]

Does this -EPERM actually close the teardown direction described in the
commit message?  The same mutation of the real device's port looks reachable
via RTM_DELLINK with CAP_NET_ADMIN only in the rmnet device's netns.

rtnl_dellink() resolves the victim in tgt_net and checks nothing against
dev_net(real_dev):

net/core/rtnetlink.c:rtnl_dellink() {
	...
	if (ifm->ifi_index > 0)
		dev = __dev_get_by_index(tgt_net, ifm->ifi_index);
	...
	if (dev)
		err = rtnl_delete_link(dev, portid, nlh);
}

rmnet_dellink() then reaches into the real device's port unconditionally:

drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c:rmnet_dellink() {
	real_port = rmnet_get_port_rtnl(real_dev);
	bridge_dev = real_port->bridge_ep;
	if (bridge_dev) {
		bridge_port = rmnet_get_port_rtnl(bridge_dev);
		rmnet_unregister_bridge(bridge_port);
	}
	...
	netdev_upper_dev_unlink(real_dev, dev);
	rmnet_unregister_real_device(real_dev);
}

so real_port->bridge_ep and real_port->rmnet_mode are cleared, the mux
endpoint is removed, real_port->nr_rmnet_devs is decremented, and
rmnet_unregister_real_device() can call netdev_rx_handler_unregister() on
real_dev and free the port.

The precondition is the same one the patch assumes for changelink: the rmnet
device is created by an admin privileged in the real device's netns and then
moved into another netns, after which "ip link del rmnet0" from that netns
reaches the sequence above.

Deleting the bridge slave device looks similar:
rmnet_config_notify_cb() -> rmnet_force_unassociate_device() also calls
rmnet_unregister_bridge(port), which follows port->bridge_ep into the real
device's netns with no check against dev_net(real_dev).

If those paths stay ungated, is the net effect of the new check in
rmnet_del_bridge() limited to rejecting the clean IFLA_MASTER=0 unenslave,
while the commit message's statement that

  "A caller privileged only in the rmnet device's netns can therefore
   rewrite the shared cellular data-path state owned by another netns"

remains true for the teardown direction after the patch?

Teardown-on-delete is normal netdev lifecycle behaviour, and the vxlan/geneve
series cited in the commit message gated only changelink, so gating dellink
may not be the right fix.  Could the commit message state explicitly that the
dellink and notifier teardown paths are out of scope, so stable backporters
can judge the coverage?  The add direction, where bridge_ep is pointed at a
caller-owned device and rmnet_rx_handler() steers the real device's ingress
frames, does look closed by the rmnet_add_bridge() gate.

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
> index 4f0ddcedfa97..1f4a3246f254 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c

[ ... ]

> @@ -240,9 +242,23 @@ static int rmnet_set_coalesce(struct net_device *dev,
>  			      struct netlink_ext_ack *extack)
>  {
>  	struct rmnet_priv *priv = netdev_priv(dev);
> +	struct net_device *real_dev = priv->real_dev;
>  	struct rmnet_port *port;
>  
> -	port = rmnet_get_port_rtnl(priv->real_dev);
> +	/*
> +	 * The aggregation parameters live in the port attached to
> +	 * real_dev, which may reside in a different netns.  The ethtool

[Severity: Low]

This isn't a bug, but this block comment opens with a bare "/*" on its own
line, while the v4 changelog says "use the netdev comment style, per Subash
Abhinov Kasiviswanathan".

The three comments this patch adds to rmnet_config.c use the netdev form,
for example in rmnet_changelink():

	/* The rtnl path only checks CAP_NET_ADMIN against dev_net(dev),

and the existing comments in rmnet_vnd.c do too, for example above
rmnet_vnd_setup():

	/* Called by kernel whenever a new rmnet<n> device is created. Sets MTU,
	 * flags, ARP type, needed headroom, etc...
	 */

Note that checkpatch.pl in this tree has no networking block comment check,
so this is only an internal inconsistency with the rest of the patch and the
rest of the file.

> +	 * paths only require CAP_NET_ADMIN in dev's netns, so require it
> +	 * in real_dev's netns as well before mutating the shared port
> +	 * state.
> +	 */
> +	if (!ns_capable(dev_net(real_dev)->user_ns, CAP_NET_ADMIN)) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Changing aggregation parameters requires CAP_NET_ADMIN in the real device network namespace");
> +		return -EPERM;
> +	}
> +
> +	port = rmnet_get_port_rtnl(real_dev);

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