ppp: defer channel free to an RCU grace period to fix pppol2tp RX UAF

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.

Defer the channel free to an RCU callback via call_rcu() so the grace
period fences any in-flight ppp_input(). The disconnect and unbridge
teardown paths already fence with synchronize_net()/synchronize_rcu();
call_rcu() does the same here without stalling the close() path.

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>
Reviewed-by: Qingfang Deng <qingfang.deng@linux.dev>
Link: https://patch.msgid.link/E793FCF2-58DE-4387-A983-C7B4BC3158BD@doyensec.com
Signed-off-by: Paolo Abeni <pabeni@redhat.com>

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 57c68efa5ff81da1233274b66f849c0cf86b6beb..717c1d3aa95380da64eb5ae6fc1da6e4dfaa9d87 100644 (file)
--- 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 */
@@ -3562,6 +3563,18 @@ ppp_disconnect_channel(struct channel *pch)
        return err;
 }
 
+/* Purge after the grace period: a late ppp_input() may still queue an
+ * skb on pch->file.rq before the last RCU reader drains.
+ */
+static void ppp_release_channel_free(struct rcu_head *rcu)
+{
+       struct channel *pch = container_of(rcu, struct channel, rcu);
+
+       skb_queue_purge(&pch->file.xq);
+       skb_queue_purge(&pch->file.rq);
+       kfree(pch);
+}
+
 /*
  * Drop a reference to a ppp channel and free its memory if the refcount reaches
  * zero.
@@ -3581,9 +3594,7 @@ static void ppp_release_channel(struct channel *pch)
                pr_err("ppp: destroying undead channel %p !\n", pch);
                return;
        }
-       skb_queue_purge(&pch->file.xq);
-       skb_queue_purge(&pch->file.rq);
-       kfree(pch);
+       call_rcu(&pch->rcu, ppp_release_channel_free);
 }
 
 static void __exit ppp_cleanup(void)
@@ -3596,6 +3607,7 @@ static void __exit ppp_cleanup(void)
        device_destroy(&ppp_class, MKDEV(PPP_MAJOR, 0));
        class_unregister(&ppp_class);
        unregister_pernet_device(&ppp_net_ops);
+       rcu_barrier(); /* wait for RCU callbacks before module unload */
 }
 
 /*