[PATCH net] ppp: defer channel free to an RCU grace period to fix pppol2tp RX UAF

[PATCH net] ppp: defer channel free to an RCU grace period to fix pppol2tp RX UAF

[Norbert Szetei]

pppol2tp_recv() runs in the L2TP UDP-encap softirq RX path:

 l2tp_udp_encap_recv() -> l2tp_recv_common() -> pppol2tp_recv()
   -> ppp_input(&po->chan)

It runs under rcu_read_lock() holding only an l2tp_session reference and
takes NO reference on the internal PPP channel (struct channel,
chan->ppp) that ppp_input() dereferences.

The pppox socket is SOCK_RCU_FREE, so 'po' and the embedded ppp_channel
are RCU-safe.  But the internal struct channel is a separate allocation
that ppp_release_channel() frees with a plain kfree():

 close(data socket) -> pppol2tp_release() -> pppox_unbind_sock()
   -> ppp_unregister_channel() -> ppp_release_channel() -> kfree(pch)

For a channel that is bound (PPPIOCGCHAN) but not attached to a ppp unit
(no PPPIOCCONNECT, pch->ppp == NULL) and not bridged, teardown skips
both ppp_disconnect_channel()'s synchronize_net() and
ppp_unbridge_channels()'s synchronize_rcu(), so the kfree() has no grace
period.  rcu_read_lock() in pppol2tp_recv() does not protect against a
plain kfree(), so an in-flight ppp_input() on one CPU can dereference
the channel just freed by close() on another CPU.

The bug is reachable by an unprivileged user.

Free the channel with kfree_rcu() instead of kfree() so the grace period
fences any in-flight ppp_input(). Done in ppp_release_channel(), this
covers all callers in one place.

Fixes: ee40fb2e1eb5 ("l2tp: protect sock pointer of struct pppol2tp_session with RCU")
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Norbert Szetei <norbert@doyensec.com>
---
 drivers/net/ppp/ppp_generic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 57c68efa5ff8..cb8fe37170d3 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -184,6 +184,7 @@ struct channel {
 	struct list_head clist;		/* link in list of channels per unit */
 	spinlock_t	upl;		/* protects `ppp' and 'bridge' */
 	struct channel __rcu *bridge;	/* "bridged" ppp channel */
+	struct rcu_head	rcu;		/* for RCU-deferred free of the channel */
 #ifdef CONFIG_PPP_MULTILINK
 	u8		avail;		/* flag used in multilink stuff */
 	u8		had_frag;	/* >= 1 fragments have been sent */
@@ -3583,7 +3584,7 @@ static void ppp_release_channel(struct channel *pch)
 	}
 	skb_queue_purge(&pch->file.xq);
 	skb_queue_purge(&pch->file.rq);
-	kfree(pch);
+	kfree_rcu(pch, rcu);
 }
 
 static void __exit ppp_cleanup(void)
-- 
2.54.0

Re: [PATCH net] ppp: defer channel free to an RCU grace period to fix pppol2tp RX UAF

[Breno Leitao]

On Wed, Jul 01, 2026 at 02:14:39PM +0200, Norbert Szetei wrote:
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index 57c68efa5ff8..cb8fe37170d3 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -184,6 +184,7 @@ struct channel {
>  	struct list_head clist;		/* link in list of channels per unit */
>  	spinlock_t	upl;		/* protects `ppp' and 'bridge' */
>  	struct channel __rcu *bridge;	/* "bridged" ppp channel */
> +	struct rcu_head	rcu;		/* for RCU-deferred free of the channel */
>  #ifdef CONFIG_PPP_MULTILINK
>  	u8		avail;		/* flag used in multilink stuff */
>  	u8		had_frag;	/* >= 1 fragments have been sent */
> @@ -3583,7 +3584,7 @@ static void ppp_release_channel(struct channel *pch)
>  	}
>  	skb_queue_purge(&pch->file.xq);
>  	skb_queue_purge(&pch->file.rq);
> -	kfree(pch);
> +	kfree_rcu(pch, rcu);

Why not use kfree_rcu_mightsleep() instead? That would eliminate the need
for the additional `struct rcu_head rcu;` field.

Re: [PATCH net] ppp: defer channel free to an RCU grace period to fix pppol2tp RX UAF

[Norbert Szetei]

On Jul 1, 2026, at 15:15, Breno Leitao <leitao@debian.org> wrote:
> 
> On Wed, Jul 01, 2026 at 02:14:39PM +0200, Norbert Szetei wrote:
>> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
>> index 57c68efa5ff8..cb8fe37170d3 100644
>> --- a/drivers/net/ppp/ppp_generic.c
>> +++ b/drivers/net/ppp/ppp_generic.c
>> @@ -184,6 +184,7 @@ struct channel {
>> struct list_head clist; /* link in list of channels per unit */
>> spinlock_t upl; /* protects `ppp' and 'bridge' */
>> struct channel __rcu *bridge; /* "bridged" ppp channel */
>> + struct rcu_head rcu; /* for RCU-deferred free of the channel */
>> #ifdef CONFIG_PPP_MULTILINK
>> u8 avail; /* flag used in multilink stuff */
>> u8 had_frag; /* >= 1 fragments have been sent */
>> @@ -3583,7 +3584,7 @@ static void ppp_release_channel(struct channel *pch)
>> }
>> skb_queue_purge(&pch->file.xq);
>> skb_queue_purge(&pch->file.rq);
>> - kfree(pch);
>> + kfree_rcu(pch, rcu);
> 
> Why not use kfree_rcu_mightsleep() instead? That would eliminate the need
> for the additional `struct rcu_head rcu;` field.

You are right, kfree_rcu_mightsleep() would be simpler, but it's
free-only. No callback to run the deferred purge, it can't handle the
in-flight skb. So, I'll keep the rcu_head and use call_rcu() in v2.

Re: [PATCH net] ppp: defer channel free to an RCU grace period to fix pppol2tp RX UAF

[Sebastian Andrzej Siewior]

On 2026-07-01 14:14:39 [+0200], Norbert Szetei wrote:
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -184,6 +184,7 @@ struct channel {
>  	struct list_head clist;		/* link in list of channels per unit */
>  	spinlock_t	upl;		/* protects `ppp' and 'bridge' */
>  	struct channel __rcu *bridge;	/* "bridged" ppp channel */
> +	struct rcu_head	rcu;		/* for RCU-deferred free of the channel */
>  #ifdef CONFIG_PPP_MULTILINK
>  	u8		avail;		/* flag used in multilink stuff */
>  	u8		had_frag;	/* >= 1 fragments have been sent */
> @@ -3583,7 +3584,7 @@ static void ppp_release_channel(struct channel *pch)
>  	}
>  	skb_queue_purge(&pch->file.xq);
>  	skb_queue_purge(&pch->file.rq);
> -	kfree(pch);
> +	kfree_rcu(pch, rcu);

From looking at ppp_input(), what ensures that the skb in-flight is not
added skb_queue which is purged above?

>  }

Sebastian

Re: [PATCH net] ppp: defer channel free to an RCU grace period to fix pppol2tp RX UAF

[Norbert Szetei]

On Jul 1, 2026, at 15:25, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> 
> On 2026-07-01 14:14:39 [+0200], Norbert Szetei wrote:
>> --- a/drivers/net/ppp/ppp_generic.c
>> +++ b/drivers/net/ppp/ppp_generic.c
>> @@ -184,6 +184,7 @@ struct channel {
>> struct list_head clist; /* link in list of channels per unit */
>> spinlock_t upl; /* protects `ppp' and 'bridge' */
>> struct channel __rcu *bridge; /* "bridged" ppp channel */
>> + struct rcu_head rcu; /* for RCU-deferred free of the channel */
>> #ifdef CONFIG_PPP_MULTILINK
>> u8 avail; /* flag used in multilink stuff */
>> u8 had_frag; /* >= 1 fragments have been sent */
>> @@ -3583,7 +3584,7 @@ static void ppp_release_channel(struct channel *pch)
>> }
>> skb_queue_purge(&pch->file.xq);
>> skb_queue_purge(&pch->file.rq);
>> - kfree(pch);
>> + kfree_rcu(pch, rcu);
> 
> From looking at ppp_input(), what ensures that the skb in-flight is not
> added skb_queue which is purged above?

Good catch, purging before the free races an in-flight ppp_input() and
leaks the skb, confirmed with kmemleak. In v2 I moved the purge into the
call_rcu() callback so it runs after the grace period.

N.

> 
>> }
> 
> Sebastian