Re: [PATCH net v2] macvlan: require lower-netns admin for shared port settings

On 8/2/26 3:01 PM, Doruk Tan Ozturk wrote:
> struct macvlan_port is per lower device and is shared by every macvlan
> upper on it, including uppers that live in other network namespaces.
> Two of its fields are settable over rtnetlink by any upper on the port:
> port->bc_cutoff, written by IFLA_MACVLAN_BC_CUTOFF, and
> port->bc_queue_len_used, recomputed from IFLA_MACVLAN_BC_QUEUE_LEN.
> (port->flags and port->perm_addr are also rtnetlink-settable, but only
> in passthru mode, which requires port->count == 0 and so cannot be
> reached from a second upper.)
> 
> rtnetlink checks CAP_NET_ADMIN against the network namespace the
> configured device lives in and nothing else, so once a macvlan has been
> moved into a child network namespace, an administrator of that namespace
> alone reaches macvlan_changelink(), which applies both attributes
> without considering who owns the lower device.
> 
> The create path has the same gap. macvlan_common_newlink() resolves a
> lower device that is itself a macvlan to the real lower device:
> 
> 	if (netif_is_macvlan(lowerdev))
> 		lowerdev = macvlan_dev_real_dev(lowerdev);
> 
> That real device may sit in a network namespace that was never
> capability-checked. The new upper then joins its macvlan_port and runs
> update_port_bc_queue_len() on it, and, when IFLA_MACVLAN_BC_CUTOFF is
> present, update_port_bc_cutoff().
> 
> port->bc_cutoff is not a local tuning knob. update_port_bc_cutoff()
> recomputes port->bc_filter, which macvlan_handle_frame() tests to decide
> whether a multicast frame is deferred to the port broadcast work queue
> or flooded inline from the RX softirq, and a negative cutoff clears
> bc_filter outright. A namespace that administers none of the other
> uppers can therefore change how all of them receive multicast.
> 
> Reproduced on 6.8 with a dummy lower device and two macvlan uppers, one
> left in the initial namespace and one moved into a child user and
> network namespace. From the child, both a changelink and a nested
> newlink carrying IFLA_MACVLAN_BC_CUTOFF were accepted, and the value
> read back on the initial-namespace sibling followed them, changing from
> 1 to -7 and then to -42.
> 
> Require CAP_NET_ADMIN in the lower device network namespace before
> applying a shared port setting or creating a macvlan on a flattened
> lower device. 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").
> 
> Found by 0sec automated security-research tooling (https://0sec.ai).
> 
> The newlink gate is unconditional rather than keyed on a BC attribute
> being present, because joining another namespace's macvlan_port is
> itself a mutation of shared state; ipvlan gates ipvlan_link_new() the
> same way.
> 
> IFLA_MACVLAN_BC_QUEUE_LEN is gated here as well as by any magnitude
> check, because the two address different things: a magnitude check
> bounds how large a value any caller may request, while this bounds who
> may write the shared port at all. update_port_bc_queue_len() takes the
> maximum across uppers, so a cross-namespace lowering has no security
> effect and this over-rejects it; that is accepted in exchange for one
> rule covering every writer of the shared struct.
> 
> Fixes: d4bff72c8401 ("macvlan: Support for high multicast packet rate")
> Fixes: 954d1fa1ac93 ("macvlan: Add netlink attribute for broadcast cutoff")
> Cc: [email protected]
> Assisted-by: 0sec:multi-model
> Signed-off-by: Doruk Tan Ozturk <[email protected]>
I think that following ipvlan example is correct, but the behavior
change may break existing user; we don't want bad regression this late.
I think this is more suitable for net-next, with no fixes tag.

/P