Re: [PATCH net] macvlan: require lower-netns admin for IFLA_MACVLAN_FLAGS

[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 (2) · 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] macvlan: incomplete CAP_NET_ADMIN check allows traffic
  interception
- [Medium] The commit message presents commit 5b782a811e73 ("macvlan:
  require lower-netns admin for shared port settings") as covering…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  macvlan_changelink() (drivers/net/macvlan.c:1665-1678) calls…
- [Medium] This problem wasn't introduced by this patch, but the new
  check relies on rtnl_dev_link_net_capable()…

--

macvlan: require lower-netns admin for IFLA_MACVLAN_FLAGS

This patch extends the rtnl_dev_link_net_capable() check in
macvlan_changelink() to IFLA_MACVLAN_FLAGS. Changing that attribute now
requires CAP_NET_ADMIN in the lower device's network namespace. The
reason given is that toggling MACVLAN_FLAG_NOPROMISC in passthru mode
calls dev_set_promiscuity() on the lower device.

> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index afad90b9222a2..d30cffddb1e1b 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -1641,7 +1641,8 @@ static int macvlan_changelink(struct net_device *dev,
>  	int ret;
>  
>  	if (data &&
> -	    (data[IFLA_MACVLAN_BC_QUEUE_LEN] || data[IFLA_MACVLAN_BC_CUTOFF]) &&
> +	    (data[IFLA_MACVLAN_BC_QUEUE_LEN] || data[IFLA_MACVLAN_BC_CUTOFF] ||
> +	     data[IFLA_MACVLAN_FLAGS]) &&
>  	    !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");

[Severity: High]
This isn't introduced by this patch. Since the guard is being
extended here, though, does it still miss the attributes that let a
child netns admin receive traffic from the lower device?

IFLA_MACVLAN_MODE and IFLA_MACVLAN_MACADDR_MODE are not covered by the
rtnl_dev_link_net_capable() check. One changelink request is enough
for root in the macvlan's netns to do both of these:

  1. Switch a non-passthru macvlan to MACVLAN_MODE_SOURCE. set_mode is
     applied before the IFLA_MACVLAN_MACADDR_MODE branch.
  2. Pass MACVLAN_MACADDR_ADD with an arbitrary MAC address.

macvlan_changelink_sources() then calls macvlan_hash_add_source().
That inserts the entry into vlan->port->vlan_source_hash, which
belongs to the shared struct macvlan_port, not to the upper device.

On receive, macvlan_handle_frame() calls macvlan_forward_source() for
every frame on the lower device, keyed on eth->h_source. For each
matching entry, macvlan_forward_source_one() delivers a clone to that
vlan. Nothing restricts the MAC to stations that belong to the child
netns.

A container that only owns the upper device could therefore add the
MAC of, for example, the gateway on that segment. It would then get a
copy of every frame that station sends on the lower device. That
includes frames addressed to the lower device itself or to macvlans in
other namespaces.

If MACVLAN_FLAG_NODST is already set on that vlan, consume becomes
true in macvlan_forward_source(). The frame is then dropped instead of
reaching its real destination.

5b782a811e73 treated mode as a per-upper setting. Does that still hold
once MACVLAN_MODE_SOURCE fills a port-wide hash?

Should the check also cover IFLA_MACVLAN_MACADDR_MODE, and maybe the
switch into MACVLAN_MODE_SOURCE? Or is source-mode filtering meant to
be allowed for an admin of the upper's namespace alone?

[Severity: Medium]

The commit message says that 5b782a811e73 "only covered
IFLA_MACVLAN_BC_QUEUE_LEN and IFLA_MACVLAN_BC_CUTOFF". That reads as if
flags were overlooked. However, 5b782a811e73 left flags ungated on
purpose:

    rtnl_dev_link_net_capable() short-circuits when the lower
    device shares the macvlan network namespace, so an ordinary
    single-namespace configuration is unaffected, and per-upper settings
    such as mode and flags stay available to an administrator of the
    macvlan's own namespace. This is the model ipvlan has used since
    commit 7cc9f7003a96 ("ipvlan: disallow userns cap_net_admin to change
    global mode/flags").

That commit also said the passthru-only port state cannot be reached
from a second upper, since passthru requires port->count == 0.

Should the commit message say that this reverses the earlier decision,
and explain why that reasoning no longer holds?

The extack text is reused unchanged. IFLA_MACVLAN_FLAGS is stored in
vlan->flags in struct macvlan_dev, not in the shared struct
macvlan_port. Is "Changing shared macvlan port settings" accurate for
this attribute?

The new check also covers more than the problem described. In the flags
branch below, only this path touches the lower device:

	if (macvlan_passthru(vlan->port) && promisc) {
		...
			err = dev_set_promiscuity(vlan->lowerdev, -1);
		...
	}
	WRITE_ONCE(vlan->flags, flags);

On a non-passthru macvlan, or for a change that leaves NOPROMISC alone
(for example a NODST-only change), the branch only writes vlan->flags.
After this patch, an admin of the macvlan's own netns without
CAP_NET_ADMIN in the lower netns gets -EPERM for those changes too.

Is this uAPI change intended? Could the check be limited to passthru
mode when MACVLAN_FLAG_NOPROMISC actually changes?

[Severity: Medium]

This isn't a bug introduced by this patch, but the IFLA_MACVLAN_FLAGS
branch in macvlan_changelink() changes the lower device's promiscuity
without checking netif_running(dev):

	if (data && data[IFLA_MACVLAN_FLAGS]) {
		__u16 flags = nla_get_u16(data[IFLA_MACVLAN_FLAGS]);
		bool promisc = (flags ^ vlan->flags) & MACVLAN_FLAG_NOPROMISC;
		if (macvlan_passthru(vlan->port) && promisc) {
			int err;

			if (flags & MACVLAN_FLAG_NOPROMISC)
				err = dev_set_promiscuity(vlan->lowerdev, -1);
			else
				err = dev_set_promiscuity(vlan->lowerdev, 1);

Otherwise the passthru promiscuity reference is held only while the
macvlan is up. macvlan_open() takes it:

	if (macvlan_passthru(vlan->port)) {
		if (!(vlan->flags & MACVLAN_FLAG_NOPROMISC)) {
			err = dev_set_promiscuity(lowerdev, 1);

macvlan_stop() drops it:

	if (macvlan_passthru(vlan->port)) {
		if (!(vlan->flags & MACVLAN_FLAG_NOPROMISC))
			dev_set_promiscuity(lowerdev, -1);

What happens if NOPROMISC is toggled while the macvlan is down?

Clearing NOPROMISC while down adds 1. A later open adds another 1, but
stop removes only one. Does the lower device then stay promiscuous for
good?

Setting NOPROMISC while down drops a reference that was never taken.
That could take away another holder's reference. If the counter was 0,
__dev_set_promiscuity() would wrap it to UINT_MAX with IFF_PROMISC set,
and later +1 requests would fail with -EOVERFLOW.

This goes back to df8ef8f3aaa6. With this patch applied, reaching this
path needs CAP_NET_ADMIN in the lower netns, or both devices in the
same netns.

[Severity: Medium]

This is a pre-existing issue, but the check extended here depends on
rtnl_dev_link_net_capable(), which only looks at current's credentials:

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);
}

The other cross-netns checks in rtnetlink use
netlink_ns_capable(skb, ...). That also requires the socket opener to
hold the capability (the CVE-2014-0181 hardening).

Could a confused deputy get past the lower-netns check here? For
example:

  1. An unprivileged user who owns a child userns opens a NETLINK_ROUTE
     socket in the child netns.
  2. A privileged program (for example a setuid-root one) is made to
     write an attacker-controlled RTM_NEWLINK message to that fd.
  3. netlink_net_capable() in rtnetlink_rcv_msg() passes, because the
     opener is privileged in the child userns.
  4. rtnl_dev_link_net_capable() in macvlan_changelink() passes,
     because current is global root.

Nothing checks that the opener lacks CAP_NET_ADMIN in the lower netns.

The helper came from 8165f7ff57d966. Other callers such as ip_gre,
vxlan and geneve share it, so a fix there would probably be a separate
patch.

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