Re: [syzbot] [net?] KASAN: slab-use-after-free Write in l2tp_session_delete

Hillf Danton
2024-07-03T11:52:06Z

On Wed, 3 Jul 2024 12:24:49 +0100 Tom Parkin <tparkin@katalix.com>
>
> [-- Attachment #1.1: Type: text/plain, Size: 379 bytes --]
>
> On Tue, Jun 25, 2024 at 06:25:23 -0700, syzbot wrote:
> > syzbot found the following issue on:
> >
> > HEAD commit: 185d72112b95 net: xilinx: axienet: Enable multicast by def..
> > git tree: net-next
> > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1062bd46980000
>
> #syz test https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git 185d72112b95
>
> [-- Attachment #1.2: 0001-l2tp-fix-possible-UAF-when-cleaning-up-tunnels.patch --]
> [-- Type: text/x-diff, Size: 3275 bytes --]
>
> From 31321b7742266c4e58355076c19d8d490fa005d2 Mon Sep 17 00:00:00 2001
> From: James Chapman <jchapman@katalix.com>
> Date: Tue, 2 Jul 2024 12:49:07 +0100
> Subject: [PATCH] l2tp: fix possible UAF when cleaning up tunnels
>
> syzbot reported a UAF caused by a race when the L2TP work queue closes a
> tunnel at the same time as a userspace thread closes a session in that
> tunnel.
>
> Tunnel cleanup is handled by a work queue which iterates through the
> sessions contained within a tunnel, and closes them in turn.
>
> Meanwhile, a userspace thread may arbitrarily close a session via
> either netlink command or by closing the pppox socket in the case of
> l2tp_ppp.
>
> The race condition may occur when l2tp_tunnel_closeall walks the list
> of sessions in the tunnel and deletes each one. Currently this is
> implemented using list_for_each_safe, but because the list spinlock is
> dropped in the loop body it's possible for other threads to manipulate
> the list during list_for_each_safe's list walk. This can lead to the
> list iterator being corrupted, leading to list_for_each_safe spinning.
> One sequence of events which may lead to this is as follows:
>
> * A tunnel is created, containing two sessions A and B.
> * A thread closes the tunnel, triggering tunnel cleanup via the work
> queue.
> * l2tp_tunnel_closeall runs in the context of the work queue. It
> removes session A from the tunnel session list, then drops the list
> lock. At this point the list_for_each_safe temporary variable is
> pointing to the other session on the list, which is session B, and
> the list can be manipulated by other threads since the list lock has
> been released.
> * Userspace closes session B, which removes the session from its parent
> tunnel via l2tp_session_delete. Since l2tp_tunnel_closeall has
> released the tunnel list lock, l2tp_session_delete is able to call
> list_del_init on the session B list node.
> * Back on the work queue, l2tp_tunnel_closeall resumes execution and
> will now spin forever on the same list entry until the underlying
> session structure is freed, at which point UAF occurs.
>
> The solution is to iterate over the tunnel's session list using
> list_first_entry_not_null to avoid the possibility of the list
> iterator pointing at a list item which may be removed during the walk.
>
> ---
> net/l2tp/l2tp_core.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index 64f446f0930b..afa180b7b428 100644
> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c
> @@ -1290,13 +1290,14 @@ static void l2tp_session_unhash(struct l2tp_session *session)
> static void l2tp_tunnel_closeall(struct l2tp_tunnel *tunnel)
> {
> struct l2tp_session *session;
> - struct list_head *pos;
> - struct list_head *tmp;
>
> spin_lock_bh(&tunnel->list_lock);
> tunnel->acpt_newsess = false;
> - list_for_each_safe(pos, tmp, &tunnel->session_list) {
> - session = list_entry(pos, struct l2tp_session, list);
> + for (;;) {
> + session = list_first_entry_or_null(&tunnel->session_list,
> + struct l2tp_session, list);
> + if (!session)
> + break;

WTF difference could this patch make wrt closing the race above?

> list_del_init(&session->list);
> spin_unlock_bh(&tunnel->list_lock);
> l2tp_session_delete(session);