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

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?

[ ... ]

[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