[PATCH net v1] net: phy: fix NULL deref in IRQ handler after unbind

Xuanqiang Luo <[email protected]>
Newsgroups org.kernel.vger.stable,org.kernel.vger.linux-kernel,org.kernel.vger.netdev
Message-ID <[email protected]>
From: Xuanqiang Luo <[email protected]>

phydev->drv can become NULL while the phy_device is still attached to
its net_device, after the PHY driver is unbound via sysfs:

    echo <mdio_id> > /sys/bus/mdio_bus/drivers/<phy_drv>/unbind

phy_remove() clears phydev->drv but does not free the threaded IRQ
installed at attach time. A later hardware interrupt, or another
device on the same IRQF_SHARED line, then oopses in phy_interrupt()
on phydev->drv->handle_interrupt():

    Unable to handle kernel NULL pointer dereference at 0x138
    ...
    pc : phy_interrupt+0xb4/0x108
    Call trace:
     phy_interrupt+0xb4/0x108 (P)
     irq_thread_fn+0x34/0xb8
     irq_thread+0xc8/0x178
     kthread+0x128/0x138
     ret_from_fork+0x10/0x20

The same unbind sequence is already treated as valid. Skip the
driver callback when no PHY driver is bound, matching other phylib
unbind paths. Also guard phy_config_interrupt(), which is reached
from phy_disconnect() after the same unbind.

The IRQ is requested at attach time and released by phy_disconnect(),
so phy_remove() cannot free it without a later double-free. After
this change the handler returns IRQ_NONE. If the PHY keeps the line
asserted and no other shared handler claims it, the IRQ core may
eventually disable the whole line ("nobody cared") and affect other
devices on that IRQ. That is a limitation of not tearing the IRQ
down on driver unbind, but it is still better than the NULL pointer
dereference that the same interrupt triggers today.

Fixes: c974bdbc3e77 ("net: phy: Use threaded IRQ, to allow IRQ from sleeping devices")
Cc: [email protected]
Signed-off-by: Xuanqiang Luo <[email protected]>
---
 drivers/net/phy/phy.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index fce9bc7be3304..5435f62e98171 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -168,7 +168,7 @@ EXPORT_SYMBOL_GPL(phy_get_rate_matching);
 static int phy_config_interrupt(struct phy_device *phydev, bool interrupts)
 {
 	phydev->interrupts = interrupts ? 1 : 0;
-	if (phydev->drv->config_intr)
+	if (phydev->drv && phydev->drv->config_intr)
 		return phydev->drv->config_intr(phydev);
 
 	return 0;
@@ -1436,7 +1436,10 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
 	}
 
 	mutex_lock(&phydev->lock);
-	ret = phydev->drv->handle_interrupt(phydev);
+	if (!phydev->drv)
+		ret = IRQ_NONE;
+	else
+		ret = phydev->drv->handle_interrupt(phydev);
 	mutex_unlock(&phydev->lock);
 
 	return ret;
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.