[PATCH net] net/wan/hdlc_ppp: sync per-proto timers before freeing hdlc state

[PATCH net] net/wan/hdlc_ppp: sync per-proto timers before freeing hdlc state

[Fan Wu]

Each PPP control protocol (LCP/IPCP/IPV6CP) embedded in struct ppp
registers a timer via timer_setup(). That struct ppp is the
hdlc->state allocation, which detach_hdlc_protocol() frees with kfree()
in both teardown paths: unregister_hdlc_device() and the re-attach inside
attach_hdlc_protocol().

The ppp proto never registered a .detach callback, so
detach_hdlc_protocol() performs no timer synchronization before the
kfree(). The only cancel, timer_delete(&proto->timer) in ppp_cp_event(),
is partial (it does not wait for a running callback) and only runs on the
->CLOSED transition; ppp_stop()/ppp_close() do not sync either. A
ppp_timer callback already executing (blocked on ppp->lock) survives the
kfree and then dereferences proto->state / ppp->lock in freed memory,
leading to a use-after-free.

Fix this by adding a .detach helper that calls timer_shutdown_sync() on
every per-proto timer. detach_hdlc_protocol() invokes proto->detach(dev)
before kfree(hdlc->state), so timer_shutdown_sync()
now runs on both free paths.
timer_shutdown_sync() is used instead of timer_delete_sync() because the
keepalive path re-arms the timer through add_timer()/mod_timer() and
shutdown blocks any re-activation during teardown.

Initialize the per-protocol timers in ppp_ioctl() when the protocol is
attached, and remove the now-redundant timer_setup() from ppp_start(), so
that the timers are initialized exactly once at attach time and
ppp_timer_release() never operates on uninitialized timer_list
structures. attach_hdlc_protocol() uses kmalloc() (not kzalloc), so
struct ppp's protos[i].timer is uninitialized garbage until the first
timer_setup(); without this init-at-attach, attaching the PPP protocol
without ever bringing the device up would leave timer_shutdown_sync()
operating on uninitialized memory in .detach. Moving the init out of
ppp_start() (which only runs on NETDEV_UP) into the attach path makes the
initialization unconditional and avoids initializing the same timer_list
twice.

This bug was found by static analysis.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 drivers/net/wan/hdlc_ppp.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wan/hdlc_ppp.c b/drivers/net/wan/hdlc_ppp.c
index 159295c4bd6d820ca51bb768a70437b541cb3f1e..302ed27944e7c8586a04f70ea639f2da0f7dcbb9 100644
--- a/drivers/net/wan/hdlc_ppp.c
+++ b/drivers/net/wan/hdlc_ppp.c
@@ -619,7 +619,6 @@ static void ppp_start(struct net_device *dev)
 		struct proto *proto = &ppp->protos[i];

 		proto->dev = dev;
-		timer_setup(&proto->timer, ppp_timer, 0);
 		proto->state = CLOSED;
 	}
 	ppp->protos[IDX_LCP].pid = PID_LCP;
@@ -639,6 +638,15 @@ static void ppp_close(struct net_device *dev)
 	ppp_tx_flush();
 }

+static void ppp_timer_release(struct net_device *dev)
+{
+	struct ppp *ppp = get_ppp(dev);
+	int i;
+
+	for (i = 0; i < IDX_COUNT; i++)
+		timer_shutdown_sync(&ppp->protos[i].timer);
+}
+
 static struct hdlc_proto proto = {
 	.start		= ppp_start,
 	.stop		= ppp_stop,
@@ -647,6 +655,7 @@ static struct hdlc_proto proto = {
 	.ioctl		= ppp_ioctl,
 	.netif_rx	= ppp_rx,
 	.module		= THIS_MODULE,
+	.detach		= ppp_timer_release,
 };

 static const struct header_ops ppp_header_ops = {
@@ -657,7 +666,7 @@ static int ppp_ioctl(struct net_device *dev, struct if_settings *ifs)
 {
 	hdlc_device *hdlc = dev_to_hdlc(dev);
 	struct ppp *ppp;
-	int result;
+	int i, result;

 	switch (ifs->type) {
 	case IF_GET_PROTO:
@@ -685,6 +694,8 @@ static int ppp_ioctl(struct net_device *dev, struct if_settings *ifs)
 			return result;

 		ppp = get_ppp(dev);
+		for (i = 0; i < IDX_COUNT; i++)
+			timer_setup(&ppp->protos[i].timer, ppp_timer, 0);
 		spin_lock_init(&ppp->lock);
 		ppp->req_timeout = 2;
 		ppp->cr_retries = 10;
--
2.39.2

Re: [PATCH net] net/wan/hdlc_ppp: sync per-proto timers before freeing hdlc state

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 17 Jun 2026 02:05:18 +0000 you wrote:
> Each PPP control protocol (LCP/IPCP/IPV6CP) embedded in struct ppp
> registers a timer via timer_setup(). That struct ppp is the
> hdlc->state allocation, which detach_hdlc_protocol() frees with kfree()
> in both teardown paths: unregister_hdlc_device() and the re-attach inside
> attach_hdlc_protocol().
> 
> The ppp proto never registered a .detach callback, so
> detach_hdlc_protocol() performs no timer synchronization before the
> kfree(). The only cancel, timer_delete(&proto->timer) in ppp_cp_event(),
> is partial (it does not wait for a running callback) and only runs on the
> ->CLOSED transition; ppp_stop()/ppp_close() do not sync either. A
> ppp_timer callback already executing (blocked on ppp->lock) survives the
> kfree and then dereferences proto->state / ppp->lock in freed memory,
> leading to a use-after-free.
> 
> [...]

Here is the summary with links:
  - [net] net/wan/hdlc_ppp: sync per-proto timers before freeing hdlc state
    https://git.kernel.org/netdev/net/c/c78a4e41ab5e

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html