This issue was identified during our ongoing static-analysis research while
reviewing kernel code.
The v2 review found two pre-existing FSL UCC HDLC bugs and asked that
the generic HDLC behavior change be separated and audited.
Patch 1 validates that an HDLC protocol is attached before starting the
UCC, avoiding a NULL protocol dereference on a premature interface open.
Patch 2 allocates the suspend backup before disabling NAPI, so allocation
failure leaves the running interface usable. Patch 3 closes active HDLC
devices before detaching their protocol and documents the audit of every
current caller, including the farsync probe unwind. Patch 4 unregisters
and frees the FSL UCC HDLC netdev before its private resources.
Patch 4 depends on patch 3 to stop an active device while its protocol is
still attached and before UCC and DMA resources are released.
These fixes and changelogs were prepared with an LLM and then checked
against the cited source paths and call ordering.
Changes since v2:
- split the generic unregister_hdlc_device() change into its own patch;
- audit all eight in-tree HDLC hardware drivers and document farsync;
- fix the NULL protocol dereference reported in uhdlc_open();
- keep NAPI enabled when the suspend backup allocation fails;
- rebase the series onto Linux 7.3-rc1.
v2: https://lore.kernel.org/r/20260803133048.42650-1-mhun512@gmail.com
review: https://lore.kernel.org/r/20260806020541.2011936-2-kuba@kernel.org
Validation:
The series applies cleanly to net commit 784450234395.
All four patches pass strict checkpatch.
No hardware runtime testing was performed.
Myeonghun Pak (4):
net: wan: fsl_ucc_hdlc: validate protocol before starting device
net: wan: fsl_ucc_hdlc: allocate suspend backup before quiescing
net: wan: hdlc: close active devices before protocol detach
net: wan: fsl_ucc_hdlc: release HDLC device on remove
drivers/net/wan/fsl_ucc_hdlc.c | 29 ++++++++++++++++-------------
drivers/net/wan/hdlc.c | 1 +
2 files changed, 17 insertions(+), 13 deletions(-)
base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
--
2.47.1
uhdlc_open() starts the UCC, IRQ and NAPI before it calls hdlc_open().
If no HDLC protocol has been attached, hdlc_open() returns -ENOSYS. The
error path then calls uhdlc_close(), which calls hdlc_close() and
dereferences hdlc->proto even though it is NULL. Bringing up a freshly
registered interface before an IF_PROTO ioctl can therefore trigger a
NULL pointer dereference.
Call hdlc_open() before enabling the hardware. Balance a successful
protocol open with hdlc_close() if requesting the IRQ then fails. This
matches peer HDLC drivers and avoids running teardown for a protocol that
never opened.
Fixes: a59addacf899 ("drivers/net: process the result of hdlc_open() and add call of hdlc_close() in uhdlc_close()")
Cc: stable@vger.kernel.org
Reported-by: Jakub Kicinski <kuba@kernel.org>
Link: https://lore.kernel.org/r/20260806020541.2011936-2-kuba@kernel.org
Co-developed-by: Ijae Kim <redacted>
Signed-off-by: Ijae Kim <redacted>
Signed-off-by: Myeonghun Pak <redacted>
---
drivers/net/wan/fsl_ucc_hdlc.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
index 809f21fb93f56..82796452e54a2 100644
--- a/drivers/net/wan/fsl_ucc_hdlc.c
+++ b/drivers/net/wan/fsl_ucc_hdlc.c
@@ -34,8 +34,6 @@
#define TDM_PPPOHT_SLIC_MAXIN
#define RX_BD_ERRORS (R_CD_S | R_OV_S | R_CR_S | R_AB_S | R_NO_S | R_LG_S)
-static int uhdlc_close(struct net_device *dev);
-
static struct ucc_tdm_info utdm_primary_info = {
.uf_info = {
.tsa = 0,@@ -705,12 +703,18 @@ static int uhdlc_open(struct net_device *dev)
hdlc_device *hdlc = dev_to_hdlc(dev);
struct ucc_hdlc_private *priv = hdlc->priv;
struct ucc_tdm *utdm = priv->utdm;
- int rc = 0;
+ int rc;
if (priv->hdlc_busy != 1) {
+ rc = hdlc_open(dev);
+ if (rc)
+ return rc;
+
if (request_irq(priv->ut_info->uf_info.irq,
- ucc_hdlc_irq_handler, 0, "hdlc", priv))
+ ucc_hdlc_irq_handler, 0, "hdlc", priv)) {
+ hdlc_close(dev);
return -ENODEV;
+ }
cecr_subblock = ucc_fast_get_qe_cr_subblock(
priv->ut_info->uf_info.ucc_num);@@ -729,13 +733,9 @@ static int uhdlc_open(struct net_device *dev)
napi_enable(&priv->napi);
netdev_reset_queue(dev);
netif_start_queue(dev);
-
- rc = hdlc_open(dev);
- if (rc)
- uhdlc_close(dev);
}
- return rc;
+ return 0;
}
static void uhdlc_memclean(struct ucc_hdlc_private *priv)
--
2.47.1
uhdlc_suspend() detaches the netdev and disables NAPI before allocating
the parameter RAM backup. If that allocation fails, suspend returns
-ENOMEM with the interface still running but NAPI disabled. The PM core
does not call resume after a failed suspend, so a later close attempts to
disable NAPI again and can wait indefinitely.
Allocate the backup before changing the runtime state. An allocation
failure then leaves the interface attached and NAPI enabled.
Fixes: c19b6d246a35 ("drivers/net: support hdlc function for QE-UCC")
Cc: stable@vger.kernel.org
Reported-by: Jakub Kicinski <kuba@kernel.org>
Link: https://lore.kernel.org/r/20260806020541.2011936-2-kuba@kernel.org
Co-developed-by: Ijae Kim <redacted>
Signed-off-by: Ijae Kim <redacted>
Signed-off-by: Myeonghun Pak <redacted>
---
drivers/net/wan/fsl_ucc_hdlc.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
index 82796452e54a2..596f4ef053636 100644
--- a/drivers/net/wan/fsl_ucc_hdlc.c
+++ b/drivers/net/wan/fsl_ucc_hdlc.c
@@ -888,6 +888,10 @@ static int uhdlc_suspend(struct device *dev)
if (!netif_running(priv->ndev))
return 0;
+ priv->ucc_pram_bak = kmalloc_obj(*priv->ucc_pram_bak);
+ if (!priv->ucc_pram_bak)
+ return -ENOMEM;
+
netif_device_detach(priv->ndev);
napi_disable(&priv->napi);
@@ -897,10 +901,6 @@ static int uhdlc_suspend(struct device *dev)
priv->gumr = ioread32be(&uf_regs->gumr);
priv->guemr = ioread8(&uf_regs->guemr);
- priv->ucc_pram_bak = kmalloc_obj(*priv->ucc_pram_bak);
- if (!priv->ucc_pram_bak)
- return -ENOMEM;
-
/* backup HDLC parameter */
memcpy_fromio(priv->ucc_pram_bak, priv->ucc_pram,
sizeof(struct ucc_hdlc_param));
--
2.47.1
ucc_hdlc_probe() registers an HDLC netdev whose private pointer refers to
the separately allocated ucc_hdlc_private object. The remove path frees
that object and its resources without unregistering or freeing the
netdev. The registered device is left with a dangling private pointer.
Unregister the HDLC device before releasing the UCC and DMA resources so
an active interface is stopped first. Free the netdev before releasing
its private object.
This patch depends on the preceding "net: wan: hdlc: close active devices
before protocol detach" fix (patch 3 of this series). Without that fix,
protocol detach clears IFF_UP before unregister can invoke ndo_stop, so
an active interface would not be stopped before its resources are freed.
Fixes: c19b6d246a35 ("drivers/net: support hdlc function for QE-UCC")
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/r/20260803133048.42650-1-mhun512@gmail.com
Link: https://lore.kernel.org/r/20260806020541.2011936-2-kuba@kernel.org
Co-developed-by: Ijae Kim <redacted>
Signed-off-by: Ijae Kim <redacted>
Signed-off-by: Myeonghun Pak <redacted>
---
drivers/net/wan/fsl_ucc_hdlc.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
index 596f4ef053636..371150efc1a65 100644
--- a/drivers/net/wan/fsl_ucc_hdlc.c
+++ b/drivers/net/wan/fsl_ucc_hdlc.c
@@ -1255,6 +1255,8 @@ static void ucc_hdlc_remove(struct platform_device *pdev)
{
struct ucc_hdlc_private *priv = dev_get_drvdata(&pdev->dev);
+ unregister_hdlc_device(priv->ndev);
+
uhdlc_memclean(priv);
if (priv->utdm && priv->utdm->si_regs) {@@ -1266,6 +1268,7 @@ static void ucc_hdlc_remove(struct platform_device *pdev)
iounmap(priv->utdm->siram);
priv->utdm->siram = NULL;
}
+ free_netdev(priv->ndev);
kfree(priv);
dev_info(&pdev->dev, "UCC based hdlc module removed\n");
--
2.47.1
Commit ff3516442768 ("WAN: HDLC: Detach protocol before unregistering
device") moved protocol detach ahead of netdev unregister so detach could
still use its state. However, detach_hdlc_protocol() calls
hdlc_setup_dev(), which clears IFF_UP. unregister_netdevice() then sees an
already-down device and skips ndo_stop, leaving an active HDLC device
running while its driver releases resources.
Close the device under RTNL while its protocol is still attached, then
keep the existing detach-before-unregister order. This runs the hardware
stop callback and the protocol close callback before their state is
released.
Audit all current users: c101, n2, pc300too, pci200syn, wanxl,
ixp4xx_hss, fsl_qmc_hdlc and farsync unregister before releasing the
resources used by their close callbacks. The farsync probe unwind can
disable interrupts and free its RX DMA buffer first, but fst_close() does
not use that buffer or require interrupts; the card is also in FST_RESET,
so fst_closeport() does not access the port hardware.
Fixes: ff3516442768 ("WAN: HDLC: Detach protocol before unregistering device")
Cc: stable@vger.kernel.org
Reported-by: Jakub Kicinski <kuba@kernel.org>
Link: https://lore.kernel.org/r/20260806020541.2011936-2-kuba@kernel.org
Co-developed-by: Ijae Kim <redacted>
Signed-off-by: Ijae Kim <redacted>
Signed-off-by: Myeonghun Pak <redacted>
---
drivers/net/wan/hdlc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/wan/hdlc.c b/drivers/net/wan/hdlc.c
index cbed10b1d862e..a38f90d3006ed 100644
--- a/drivers/net/wan/hdlc.c
+++ b/drivers/net/wan/hdlc.c
@@ -263,6 +263,7 @@ EXPORT_SYMBOL(alloc_hdlcdev);
void unregister_hdlc_device(struct net_device *dev)
{
rtnl_lock();
+ dev_close(dev);
detach_hdlc_protocol(dev);
unregister_netdevice(dev);
rtnl_unlock();--
2.47.1
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net: wan: hdlc: close active devices before protocol detach
This adds a dev_close() call to unregister_hdlc_device() so that an up
HDLC device runs its ndo_stop and protocol close callbacks before
detach_hdlc_protocol() clears IFF_UP via hdlc_setup_dev(). The
detach-before-unregister order introduced by ff3516442768 is kept.
Link: https://lore.kernel.org/r/20260806020541.2011936-2-kuba@kernel.org
quoted hunk
diff --git a/drivers/net/wan/hdlc.c b/drivers/net/wan/hdlc.c
index cbed10b1d862e..a38f90d3006ed 100644
--- a/drivers/net/wan/hdlc.c
+++ b/drivers/net/wan/hdlc.c
@@ -263,6 +263,7 @@ EXPORT_SYMBOL(alloc_hdlcdev);
void unregister_hdlc_device(struct net_device *dev)
{
rtnl_lock();
+ dev_close(dev);
[Severity: High]
This isn't a bug introduced by this patch, but the new dev_close() makes
the remove path call into ixp4xx_hss's ndo_stop, and hss_hdlc_close()
sleeps inside a spin_lock_irqsave() section:
drivers/net/wan/ixp4xx_hss.c:hss_hdlc_close() {
spin_lock_irqsave(&npe_lock, flags);
ports_open--;
qmgr_disable_irq(port->rxq);
netif_stop_queue(dev);
napi_disable(&port->napi);
...
free_irq(...);
spin_unlock_irqrestore(&npe_lock, flags);
}
napi_disable() reaches napi_disable_locked(), which begins with
might_sleep() and loops on usleep_range(20, 200); free_irq() also calls
might_sleep() and takes desc->request_mutex.
Can that produce a "sleeping function called from invalid context" splat
here, and if the scheduler is actually entered, can the CPU leave npe_lock
held while hss_hdlc_dcd_irq() -> hss_hdlc_set_carrier() spins on the same
lock?
The same path is already reachable via "ip link set hss0 down", but the
commit message lists ixp4xx_hss among the audited-safe users:
Audit all current users: c101, n2, pc300too, pci200syn, wanxl,
ixp4xx_hss, fsl_qmc_hdlc and farsync unregister before releasing the
resources used by their close callbacks.
Should the audit note this, given the remove path now runs
hss_hdlc_close() where it previously did not?
detach_hdlc_protocol(dev);
[Severity: High]
Does dev_close() close the window this relies on for the RX path?
detach_hdlc_protocol() sets hdlc->proto = NULL and kfree()s hdlc->state
while the netdev is still registered, so hdlc_rcv() can still be reached
from NET_RX softirq for frames already queued on a per-CPU
input_pkt_queue:
drivers/net/wan/hdlc.c:hdlc_rcv() {
...
BUG_ON(!hdlc->proto->netif_rx);
return hdlc->proto->netif_rx(skb);
}
The only barrier that drops backlogged skbs for the device runs later,
inside unregister_netdevice():
net/core/dev.c:unregister_netdevice_many_notify() {
...
flush_all_backlogs();
synchronize_net();
...
}
__dev_close_many() does not touch the RX backlog, so a frame handed to
netif_rx() before ndo_stop and still sitting in another CPU's queue can
reach hdlc_rcv() after hdlc->proto became NULL. The
"if (!(dev->priv_flags & IFF_WAN_HDLC))" test in hdlc_rcv() does not
filter it out, since hdlc_setup_dev() re-sets priv_flags to
IFF_WAN_HDLC.
Can that hit the BUG_ON above, or use the freed hdlc->state inside
fr_rx()/cisco_rx()/ppp_rx() if proto was already read?
[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but the
return value of detach_hdlc_protocol() is dropped while
unregister_netdevice() runs unconditionally.
detach_hdlc_protocol() returns early when a NETDEV_PRE_TYPE_CHANGE
notifier refuses the type change, skipping both release points:
drivers/net/wan/hdlc.c:detach_hdlc_protocol() {
...
if (err) {
netdev_err(dev, "Refused to change device type\n");
return err;
}
...
module_put(hdlc->proto->module);
...
kfree(hdlc->state);
...
}
The refusal is reachable: hdlc_raw_eth's raw_eth_ioctl() calls
ether_setup(), so dev->type becomes ARPHRD_ETHER and a VLAN can be
stacked on it, after which
net/8021q/vlan.c:vlan_device_event() {
case NETDEV_PRE_TYPE_CHANGE:
/* Forbid underlaying device to change its type. */
if (vlan_uses_dev(dev))
return NOTIFY_BAD;
}
returns NOTIFY_BAD. lapbether_device_event() refuses the same way.
Does this leak the kmalloc'ed hdlc->state and the hdlc_raw_eth module
reference, given every caller then calls free_netdev() and loses the only
pointer to that state?
unregister_netdevice(dev);
rtnl_unlock();
--
This is an AI-generated review.