Re: [PATCH net] vlan: prevent cross-netns MII register writes

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] The patch only partly isolates the host PHY from the VLAN's
  network namespace.

--

vlan: prevent cross-netns MII register writes

This commit makes vlan_dev_ioctl() reject SIOCSMIIREG when the VLAN
device and its real device are in different network namespaces.
SIOCGMIIPHY and SIOCGMIIREG are still forwarded across namespaces on
purpose, and writes are still allowed when both devices share a
namespace.

> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
> index bca0a3693636c..0d0904103302f 100644
> --- a/net/8021q/vlan_dev.c
> +++ b/net/8021q/vlan_dev.c
> @@ -416,9 +416,12 @@ static int vlan_dev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
>  	ifrr.ifr_ifru = ifr->ifr_ifru;
>
>  	switch (cmd) {
> +	case SIOCSMIIREG:
> +		if (!net_eq(dev_net(dev), dev_net(real_dev)))
> +			break;
> +		fallthrough;
>  	case SIOCGMIIPHY:
>  	case SIOCGMIIREG:
> -	case SIOCSMIIREG:
>  		err = dev_eth_ioctl(real_dev, &ifrr, cmd);
>  		break;

[Severity: Medium]
Can the VLAN's namespace still change host PHY state through
SIOCGMIIREG?

The commit message describes the threat as letting the VLAN namespace
"modify PHY registers on the host's real device". It also says the
patch will "Preserve MII reads across namespaces". MII reads are not
always free of side effects, though.

For phylib drivers, the read path ends in phy_mii_ioctl() in
drivers/net/phy/phy.c. That function does a raw bus read at a phy_id
and reg_num chosen by the caller:

drivers/net/phy/phy.c:phy_mii_ioctl() {
    ...
	case SIOCGMIIREG:
		if (mdio_phy_id_is_c45(mii_data->phy_id)) {
			...
			ret = mdiobus_c45_read(phydev->mdio.bus, prtad, devad,
					       mii_data->reg_num);
		} else {
			ret = mdiobus_read(phydev->mdio.bus, mii_data->phy_id,
					   mii_data->reg_num);
		}
    ...
}

The BMSR link status bit is latched low, and genphy_update_link() in
drivers/net/phy/phy_device.c depends on that latch:

	/* The link state is latched low so that momentary link
	 * drops can be detected. ...
	 */

Vendor interrupt and event status registers are also often
clear-on-read. A read from inside the container could consume a
latched link drop, or acknowledge a pending PHY interrupt, before the
host's phylib state machine or IRQ handler sees it. Would the host
then miss link-change events?

The read also isn't limited to the PHY attached to real_dev.
mii_data->phy_id can name any C22 or C45 address on the host's MDIO
bus, including other PHYs or switches unrelated to the VLAN.

The only privilege check on this path is in dev_ioctl() in
net/core/dev_ioctl.c, and it is against the VLAN namespace's user
namespace:

	case SIOCGMIIPHY:
	case SIOCGMIIREG:
	case SIOCSIFNAME:
		dev_load(net, ifr->ifr_name);
		if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
			return -EPERM;

Container root can satisfy this check once the VLAN has been moved
into its namespace.

The write side here follows the vlan_dev_hwtstamp_set()/get split,
where only set has a net_eq() check. Hardware timestamp get has no
hardware side effects, but MII reads can.

Should SIOCGMIIPHY and SIOCGMIIREG also be rejected when
dev_net(dev) and dev_net(real_dev) differ? If not, could the commit
message explain why these cross-namespace reads are acceptable?

>  	}

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