[PATCH net] l2tp: fix use-after-free in l2tp_udp_encap_recv()

[PATCH net] l2tp: fix use-after-free in l2tp_udp_encap_recv()

[Linkui Xiao]

From: Linkui Xiao <xiaolinkui@kylinos.cn>

l2tp_udp_encap_recv() derives the tunnel pointer from the session it has
just looked up, so the reference taken by l2tp_v2_session_get() or
l2tp_v3_session_get() is the only thing that keeps the tunnel alive:
l2tp_session_free() drops the tunnel reference, and l2tp_tunnel_free()
hands the tunnel to kfree_rcu().  Both error paths of that function drop
the reference before jumping to the invalid label, which still uses the
tunnel:

	tunnel = session->tunnel;

	if (version != tunnel->version) {
		l2tp_session_put(session);
		goto invalid;
	}
	...
invalid:
	atomic_long_inc(&tunnel->stats.rx_invalid);

If the session is torn down concurrently, such a put can be the last
one, and the statistics update then writes into a freed tunnel.

The frees are RCU deferred, which hides the bug on the regular path
because __udp4_lib_rcv() holds rcu_read_lock().  It does not hide it on
the backlog path: __release_sock() drops sk_lock before invoking
sk_backlog_rcv(), takes no RCU read lock and may cond_resched() between
packets, so a grace period can complete before the statistics update.

Account the packet first and move the put to the invalid label, which
can only be reached while the reference is still held, so the put is
not applied twice.

The put on the version mismatch path came later, with commit
4d10edfd1475 ("l2tp: Fix memleak in l2tp_udp_encap_recv()."), which
plugged a session leak without accounting for the tunnel going away
with the session.

Found by inspection; no reproducer.  Hitting the window needs a session
teardown racing a packet on the backlog path, and the RCU deferred frees
mean KASAN is only likely to report it with CONFIG_RCU_STRICT_GRACE_PERIOD.

Fixes: ff6a2ac23cb0 ("l2tp: refactor udp recv to lookup to not use sk_user_data")
Cc: stable@vger.kernel.org
Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
---
 net/l2tp/l2tp_core.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 7e042a3e648c..36804743c761 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1083,16 +1083,12 @@ int l2tp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 	tunnel = session->tunnel;
 
 	/* Check protocol version */
-	if (version != tunnel->version) {
-		l2tp_session_put(session);
+	if (version != tunnel->version)
 		goto invalid;
-	}
 
 	if (version == L2TP_HDR_VER_3 &&
-	    l2tp_v3_ensure_opt_in_linear(session, skb, &ptr, &optr)) {
-		l2tp_session_put(session);
+	    l2tp_v3_ensure_opt_in_linear(session, skb, &ptr, &optr))
 		goto invalid;
-	}
 
 	l2tp_recv_common(session, skb, ptr, optr, hdrflags, length);
 	l2tp_session_put(session);
@@ -1100,7 +1096,11 @@ int l2tp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 	return 0;
 
 invalid:
+	/* The session reference is the only thing keeping the tunnel alive
+	 * here, so account the invalid packet before dropping it.
+	 */
 	atomic_long_inc(&tunnel->stats.rx_invalid);
+	l2tp_session_put(session);
 
 pass:
 	/* Put UDP header back */
-- 
2.25.1

Re: [PATCH net] l2tp: fix use-after-free in l2tp_udp_encap_recv()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message describes a use-after-free that no code
  path in the current tree can trigger.

--

l2tp: fix use-after-free in l2tp_udp_encap_recv()

This patch reorders the error paths in l2tp_udp_encap_recv() so that
tunnel->stats.rx_invalid is incremented before the session reference is
dropped. The commit message says the old order could write into a freed
tunnel when packets are processed from the socket backlog, and the patch
is tagged for stable.

> The frees are RCU deferred, which hides the bug on the regular path
> because __udp4_lib_rcv() holds rcu_read_lock().  It does not hide it on
> the backlog path: __release_sock() drops sk_lock before invoking
> sk_backlog_rcv(), takes no RCU read lock and may cond_resched() between
> packets, so a grace period can complete before the statistics update.

[ ... ]

> Found by inspection; no reproducer.  Hitting the window needs a session
> teardown racing a packet on the backlog path, and the RCU deferred frees
> mean KASAN is only likely to report it with CONFIG_RCU_STRICT_GRACE_PERIOD.

[Severity: Medium]
Can the backlog path described here actually be reached?

Only one sk_backlog_rcv callback calls l2tp_udp_encap_recv(), and that is
pppol2tp_backlog_recv() in net/l2tp/l2tp_ppp.c:

static int pppol2tp_backlog_recv(struct sock *sk, struct sk_buff *skb)
{
	int rc;

	rc = l2tp_udp_encap_recv(sk, skb);
	...
}

pppol2tp_create() installs it:

	sk->sk_backlog_rcv = pppol2tp_backlog_recv;

Nothing seems to queue skbs on a PPPoL2TP socket's backlog, though.
sk_add_backlog() is called only from __sk_receive_skb(), tcp, sctp, tipc
and x25.

The callers of __sk_receive_skb() are pppoe, pptp, l2tp_ip, l2tp_ip6,
phonet and vmci. Each of them targets its own sockets, never a
PF_PPPOX / PX_PROTO_OL2TP one.

The live entry point is the UDP encap hook set up in
l2tp_tunnel_register():

	.encap_rcv = l2tp_udp_encap_recv,

That hook is called only from udp_queue_rcv_one_skb() and
udpv6_queue_rcv_one_skb(). Both run under the rcu_read_lock() taken in
ip_local_deliver_finish() and ip6_input(). NF_QUEUE reinjection runs
okfn with BH disabled, which also counts as an RCU read-side section.

The free path is:

l2tp_session_put()->l2tp_session_free()->l2tp_tunnel_put()
  ->l2tp_tunnel_free()->kfree_rcu(tunnel, rcu)

So with the old ordering, a final put could only queue the free. The
tunnel memory would stay valid until the reader leaves the RCU section.

For the same reason, can CONFIG_RCU_STRICT_GRACE_PERIOD expose this? A
strict grace period still cannot end while the receive path holds
rcu_read_lock() or has BH disabled.

The reordering itself looks correct as hardening. Would it be more
accurate to describe it that way, and to drop or justify the
Cc: stable tag, rather than present it as a reachable use-after-free?

Also, pppol2tp_backlog_recv() appears to be unreachable, and it passes a
PPPoX sk to a function that expects a UDP tunnel socket. Could it be
removed as part of a respin?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027063657.1359170-1-xiaolinkui%40126.com