netdev - [PATCH net] net: phy: Warn if phy is attached when removing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <20220816163701.1578850-1-sean.anderson@seco.com>
Date:   Tue, 16 Aug 2022 12:37:01 -0400
From:   Sean Anderson <sean.anderson@...o.com>
To:     Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org
Cc:     Paolo Abeni <pabeni@...hat.com>, Jakub Kicinski <kuba@...nel.org>,
        "David S . Miller" <davem@...emloft.net>,
        linux-kernel@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>,
        Vladimir Oltean <olteanv@...il.com>,
        Sean Anderson <sean.anderson@...o.com>
Subject: [PATCH net] net: phy: Warn if phy is attached when removing

netdevs using phylib can be oopsed from userspace in the following
manner:

$ ip link set $iface up
$ echo $(basename $(readlink /sys/class/net/$iface/phydev)) > \
      /sys/class/net/$iface/phydev/driver/unbind
$ ip link set $iface down

However, the traceback provided is a bit too late, since it does not
capture the root of the problem (unbinding the driver). It's also
possible that the memory has been reallocated if sufficient time passes
between when the phy is detached and when the netdev touches the phy
(which could result in silent memory corruption). Add a warning at the
source of the problem. A future patch could make this more robust by
calling dev_close.

Signed-off-by: Sean Anderson <sean.anderson@...o.com>
---
This is a result of discussion around my attempt to make PCS devices
proper devices [1]. Russell pointed out [2] that someone could unbind
the PCS at any time. However, the same issue applies to ethernet phys,
as well as serdes phys. Both of these can be unbound at any time, yet
no precautions are taken to avoid dereferencing a stale pointer.

As I discussed [3], we have (in general) four ways to approach this

- Just warn and accept that we are going to oops later on
- Create devlinks between the consumer/supplier
- Create a composite device composed of the consumer and its suppliers
- Add a callback (such as dev_close) and call it when the consumer goes
  away

It is (of course) also possible to rewrite phylib so that devices are
not used (preventing the phy from being unbound). However, I suspect
that this is quite undesirable.

This patch implements the first option, which, while fixing nothing, at
least provides some better debug information.

[1] https://lore.kernel.org/netdev/20220711160519.741990-1-sean.anderson@seco.com/
[2] https://lore.kernel.org/netdev/YsyPGMOiIGktUlqD@shell.armlinux.org.uk/
[3] https://lore.kernel.org/netdev/9747f8ef-66b3-0870-cbc0-c1783896b30d@seco.com/

 drivers/net/phy/phy_device.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 0c6efd792690..d75ca97f74d4 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3119,6 +3119,8 @@ static int phy_remove(struct device *dev)
 
 	cancel_delayed_work_sync(&phydev->state_queue);
 
+	WARN_ON(phydev->attached_dev);
+
 	mutex_lock(&phydev->lock);
 	phydev->state = PHY_DOWN;
 	mutex_unlock(&phydev->lock);
-- 
2.35.1.1320.gc452695387.dirty