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 > > > > [-- 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 > > 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);