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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20240703115113.2928-1-hdanton@sina.com>
Date: Wed,  3 Jul 2024 19:51:13 +0800
From: Hillf Danton <hdanton@...a.com>
To: Tom Parkin <tparkin@...alix.com>
Cc: syzbot <syzbot+c041b4ce3a6dfd1e63e2@...kaller.appspotmail.com>,
	linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org,
	James Chapman <jchapman@...alix.com>,
	syzkaller-bugs@...glegroups.com
Subject: Re: [syzbot] [net?] KASAN: slab-use-after-free Write in l2tp_session_delete

On Wed, 3 Jul 2024 12:24:49 +0100 Tom Parkin <tparkin@...alix.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@...alix.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);