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

[netdev-bot+sashiko@kernel.org]

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