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

Tom Parkin
2024-07-03T12:39:26Z

[-- Attachment #1: Type: text/plain, Size: 5728 bytes --]

Hi Hillf,

On Wed, Jul 03, 2024 at 19:51:13 +0800, Hillf Danton wrote:
> 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?
>

Sorry if the commit message isn't clear :-(

The specific UAF that syzbot hits is due to the list node that the
list_for_each_safe temporary variable points to being modified while
the list_for_each_safe walk is in process.

This is possible due to l2tp_tunnel_closeall dropping the spin lock
that protects the list mid way through the list_for_each_safe loop.
This opens the door for another thread to call list_del_init on the
node that list_for_each_safe is planning to process next, causing
l2tp_tunnel_closeall to repeatedly iterate on that node forever.

In the context of l2tp_ppp, this eventually leads to UAF because the
session structure itself is freed when the pppol2tp socket is
destroyed and the pppol2tp sk_destruct handler unrefs the session
structure to zero.

So to avoid the UAF, the list can safely be processed using a loop
which accesses the first entry in the tunnel session list under
spin lock protection, removing that entry, then dropping the lock
to call l2tp_session_delete.

FWIW, I note that syzbot has bisected this UAF to d18d3f0a24fc ("l2tp:
replace hlist with simple list for per-tunnel session list") which
changes l2tp_tunnel_closeall from a similar pattern of repeatedly
grabbing the list head under spin lock projection, to the current code
in net-next.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]