Re: [PATCH net] macsec: require CAP_NET_ADMIN in the device netns for changelink

Re: [PATCH net] macsec: require CAP_NET_ADMIN in the device netns for changelink

[netdev-bot+sashiko]

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

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

- [Medium] The new gate in macsec_changelink()
  (drivers/net/macsec.c:3963) keys on the mere presence of…
- [Low] Commit-message inaccuracy plus check placement asymmetry: the
  message asserts that changelink is "the sole way to turn the real…

--

macsec: require CAP_NET_ADMIN in the device netns for changelink

This adds a capability check to macsec_changelink() so that a request which
would reach the driver of macsec->real_dev is refused unless the caller has
CAP_NET_ADMIN in dev_net(macsec->real_dev). The gate fires when
IFLA_MACSEC_OFFLOAD is present or when the device is already offloaded.

Two questions below, one about the commit message and one about the
condition itself.

>From the commit message:

>     Scope: this closes the changelink vector only, which is the sole way to
>     turn the real device's MACsec offload on or off, and the only way to push
>     changed SecY parameters to it.

[Severity: Low]
Is "the sole way to turn the real device's MACsec offload on or off"
accurate?  macsec_update_offload() has a second caller,
macsec_upd_offload() for MACSEC_CMD_UPD_OFFLOAD, and it reaches the same
driver callbacks on real_dev:

drivers/net/macsec.c:macsec_update_offload() {
	...
	ret = offload == MACSEC_OFFLOAD_OFF ? macsec_offload(ops->mdo_del_secy, &ctx)
					    : macsec_offload(ops->mdo_add_secy, &ctx);
	...
}

That path is not an unprivileged bypass, since the op carries

	{
		.cmd = MACSEC_CMD_UPD_OFFLOAD,
		...
		.flags = GENL_ADMIN_PERM,
	},

and genl_family_rcv_msg_permissions() resolves GENL_ADMIN_PERM through
netlink_capable() against &init_user_ns, so the namespace-local
administrator this patch is about cannot invoke it.

Still, would it be clearer to say changelink is the only unprivileged path
rather than the sole path?  And would placing the check inside
macsec_update_offload() make the two callers agree, instead of gating only
the rtnl one?

> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
> index 6f9f3aceffaad..0f351e839f144 100644
> --- a/drivers/net/macsec.c
> +++ b/drivers/net/macsec.c

[ ... ]

> @@ -3959,6 +3960,13 @@ static int macsec_changelink(struct net_device *dev, struct nlattr *tb[],
>  	if (!data)
>  		return 0;
>  
> +	if ((data[IFLA_MACSEC_OFFLOAD] || macsec_is_offloaded(macsec)) &&
> +	    !rtnl_dev_link_net_capable(dev, dev_net(macsec->real_dev))) {
> +		NL_SET_ERR_MSG(extack,
> +			       "Changing a MACsec device whose real device is in another network namespace requires CAP_NET_ADMIN in that namespace");
> +		return -EPERM;
> +	}
> +

[Severity: Medium]
Does this reject a request that touches nothing on real_dev?  The first
term keys on the presence of IFLA_MACSEC_OFFLOAD, not on whether the
requested value differs from macsec->offload.

For a non-offloaded macsec device whose real_dev is in another netns, a
request carrying IFLA_MACSEC_OFFLOAD = MACSEC_OFFLOAD_OFF now returns
-EPERM, yet neither of the two boundary-crossing blocks would have run:

drivers/net/macsec.c:macsec_changelink() {
	...
	if (data[IFLA_MACSEC_OFFLOAD]) {
		offload = nla_get_u8(data[IFLA_MACSEC_OFFLOAD]);
		if (macsec->offload != offload) {		/* false */
			macsec_offload_state_change = true;
			ret = macsec_update_offload(dev, offload, extack);
	...
	/* If h/w offloading is available, propagate to the device */
	if (!macsec_offload_state_change && macsec_is_offloaded(macsec)) {	/* false */
	...
}

So "ip link set macsec0 type macsec offload off encrypt off" from inside
the container, or any tool that re-states the current offload mode
alongside local attributes, succeeded before and fails now.

The commit message gives this as the reason for the narrower condition:

>     An unconditional gate also denies the third case, which is a needless
>     uAPI change; hence the narrower condition.

Does the narrower condition still deny that same class of purely local
request?  Would comparing nla_get_u8(data[IFLA_MACSEC_OFFLOAD]) against
macsec->offload, so the gate fires only on an actual transition, keep that
case working?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/AS8P251MB000190AAD82CD30EFBC57B4DC8B42%40AS8P251MB0001.EURP251.PROD.OUTLOOK.COM

Re: [PATCH net] macsec: require CAP_NET_ADMIN in the device netns for changelink

[Jakub Kicinski]

On Thu, 10 Sep 2026 12:20:49 +0000 netdev-bot+sashiko@kernel.org wrote:
> >     An unconditional gate also denies the third case, which is a needless
> >     uAPI change; hence the narrower condition.  
> 
> Does the narrower condition still deny that same class of purely local
> request?  Would comparing nla_get_u8(data[IFLA_MACSEC_OFFLOAD]) against
> macsec->offload, so the gate fires only on an actual transition, keep that
> case working?

Sounds reasonable. Let's factor the checks out to a separate helper
function to avoid building very complex conditions in place.
-- 
pw-bot: cr

[PATCH net] macsec: require CAP_NET_ADMIN in the device netns for changelink

[Aamir Ahmed]

macsec_changelink() can reach the driver of macsec->real_dev, but the
rtnl changelink path only checks CAP_NET_ADMIN against dev_net(dev), the
macsec device's own netns. That differs from dev_net(macsec->real_dev)
once the macsec device has been moved into another network namespace,
which is the ordinary way of handing a virtual interface to a container
while the real NIC stays behind.

Two paths in macsec_changelink() cross that boundary:

  - IFLA_MACSEC_OFFLOAD -> macsec_update_offload(), which calls
    ops->mdo_add_secy() / mdo_del_secy() on macsec->real_dev.
  - the tail "propagate to the device" block, which calls
    ops->mdo_upd_secy() on macsec->real_dev whenever the device is
    already offloaded and any other attribute changed. This one needs
    no IFLA_MACSEC_OFFLOAD attribute at all.

So a network namespace administrator with no capability in the real
device's namespace can toggle the real device's MACsec offload state and
push SecY parameters into its driver.

Reproduced with netdevsim, which implements NETIF_F_HW_MACSEC:

  # privileged setup in the initial netns
  echo "0 1" > /sys/bus/netdevsim/new_device
  ip link add link eni0np1 name macsec0 type macsec offload mac
  # macsec0 is then handed to a container: moved into an unprivileged
  # user+net namespace, while eni0np1 stays in the initial netns.

  # from inside that container, with no capability in the initial netns
  ip link set macsec0 type macsec offload off     # mdo_del_secy
  ip link set macsec0 type macsec encrypt off     # mdo_upd_secy

Both succeed, and the netdevsim driver in the initial netns logs the
corresponding SecY calls.

Gate the two boundary-crossing paths with rtnl_dev_link_net_capable(),
matching the "require CAP_NET_ADMIN in the device netns for changelink"
series applied to ip_gre, ip6_gre, ipip, ip_vti, ip6_vti, ip6_tunnel,
sit, xfrm_interface, geneve, vxlan and macvlan.

The macsec_is_offloaded() term is required: without it the mdo_upd_secy
path above stays open. The check is deliberately not applied to every
attribute, so that an administrator of the macsec device's own namespace
can still configure a non-offloaded device, whose attributes are local to
it; this follows macvlan, which likewise gates only the settings that
reach the lower device.

Scope: this closes the changelink vector only, which is the sole way to
turn the real device's MACsec offload on or off, and the only way to push
changed SecY parameters to it. Several ndo paths also reach real_dev's
driver as a side effect of the container operating its own device, and
remain gated only by CAP_NET_ADMIN in dev_net(dev), as they do for the
drivers in the series above: macsec_dev_open() / macsec_dev_stop(),
macsec_common_dellink(), and macsec_set_mac_address(), which calls
mdo_upd_secy() because the SCI is derived from the MAC. Those are a
separate question.

Fixes: 3cf3227a21d1 ("net: macsec: hardware offloading infrastructure")
Cc: stable@vger.kernel.org
Signed-off-by: Aamir Ahmed <elb12345@hotmail.co.uk>
---
Found and tested with AI assistance (Claude Code). The bug was located by
auditing rtnl_link_ops.changelink handlers for the netns capability check
that ip_gre, ip6_gre, ipip, ip_vti, ip6_vti, ip6_tunnel, sit,
xfrm_interface, geneve, vxlan and macvlan recently gained; macsec and vlan
were the two that did not have it. The reproducer, the fix and the
before/after measurements below were run on a KASAN kernel under virtme-ng
with netdevsim as the offload-capable NIC.

Notes (not part of the commit message):

* Verified on netdevsim across three kernels (vulnerable / unconditional
  gate / this patch), with these cases:

    offloaded  + unpriv cross-netns "offload off"   -> denied
    offloaded  + unpriv cross-netns "encrypt off"   -> denied  (mdo_upd_secy)
    !offloaded + unpriv cross-netns "encrypt off"   -> allowed (local only)
    !offloaded + unpriv cross-netns "offload mac"   -> denied
    same-netns privileged "encrypt off"             -> allowed (no regression)
    privileged cross-netns offload toggle           -> allowed (no regression)

  An unconditional gate also denies the third case, which is a needless
  uAPI change; hence the narrower condition.

* rtnl_dev_link_net_capable() was introduced by the tunnel changelink
  series. Older stable trees may need that helper backported first, or
  the check open-coded as
    net_eq(dev_net(dev), dev_net(macsec->real_dev)) ||
    ns_capable(dev_net(macsec->real_dev)->user_ns, CAP_NET_ADMIN)

* VLAN has the same gap and is the last one in this class. Toggling
  VLAN_FLAG_GVRP/MVRP runs the GARP/MRP applicant on vlan->real_dev and
  transmits PDUs out of it. No driver callback is involved, so the impact
  is lower, but there the mutator is also reachable from the SIOCSIFVLAN
  SET_VLAN_FLAG_CMD ioctl, so the check has to go in
  vlan_dev_change_flags() rather than in the changelink handler. Sent
  separately so that the two can be reviewed independently.

  drivers/net/macsec.c | 8 ++++++++
  1 file changed, 8 insertions(+)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 6f9f3aceff..0f351e839f 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -19,6 +19,7 @@
 #include <net/gro_cells.h>
 #include <net/macsec.h>
 #include <net/dst_metadata.h>
+#include <net/rtnetlink.h>
 #include <net/netdev_lock.h>
 #include <linux/phy.h>
 #include <linux/byteorder/generic.h>
@@ -3959,6 +3960,13 @@ static int macsec_changelink(struct net_device *dev, struct nlattr *tb[],
 	if (!data)
 		return 0;
 
+	if ((data[IFLA_MACSEC_OFFLOAD] || macsec_is_offloaded(macsec)) &&
+	    !rtnl_dev_link_net_capable(dev, dev_net(macsec->real_dev))) {
+		NL_SET_ERR_MSG(extack,
+			       "Changing a MACsec device whose real device is in another network namespace requires CAP_NET_ADMIN in that namespace");
+		return -EPERM;
+	}
+
 	if (data[IFLA_MACSEC_CIPHER_SUITE] ||
 	    data[IFLA_MACSEC_ICV_LEN] ||
 	    data[IFLA_MACSEC_SCI] ||