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

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

[Hungyu Lin]

vlan_dev_ioctl() forwards SIOCSMIIREG to the underlying real
device without checking whether the VLAN device and real device
belong to the same network namespace.

A VLAN device can be moved into another network namespace while
its real device remains in the host namespace. This allows a
process with the required privileges in the VLAN device's
namespace to modify PHY registers on the host's real device.

Reject SIOCSMIIREG when the two devices belong to different
network namespaces. Preserve MII reads across namespaces and
allow MII writes when both devices belong to the same namespace.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Hungyu Lin <dennylin0707@gmail.com>
---
 net/8021q/vlan_dev.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index bca0a3693636..0d0904103302 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;
 	}
-- 
2.43.0

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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