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

[Tom Parkin <tparkin@katalix.com>]

[-- 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 --]