* Re: [syzbot] [net?] KASAN: slab-use-after-free Write in l2tp_session_delete
2024-06-25 13:25 [syzbot] [net?] KASAN: slab-use-after-free Write in l2tp_session_delete syzbot
@ 2024-06-25 14:53 ` James Chapman
2024-07-02 9:52 ` syzbot
` (4 subsequent siblings)
5 siblings, 0 replies; 15+ messages in thread
From: James Chapman @ 2024-06-25 14:53 UTC (permalink / raw)
To: syzbot, davem, edumazet, kuba, linux-kernel, netdev, pabeni,
syzkaller-bugs
On 25/06/2024 14:25, syzbot wrote:
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit: 185d72112b95 net: xilinx: axienet: Enable multicast by def..
> git tree: net-next
> console+strace: https://syzkaller.appspot.com/x/log.txt?x=129911fa980000
> kernel config: https://syzkaller.appspot.com/x/.config?x=e78fc116033e0ab7
> dashboard link: https://syzkaller.appspot.com/bug?extid=c041b4ce3a6dfd1e63e2
> compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=12521b9c980000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1062bd46980000
>
> Downloadable assets:
> disk image: https://storage.googleapis.com/syzbot-assets/e84f50e44254/disk-185d7211.raw.xz
> vmlinux: https://storage.googleapis.com/syzbot-assets/df64b575cc01/vmlinux-185d7211.xz
> kernel image: https://storage.googleapis.com/syzbot-assets/16ad5d1d433b/bzImage-185d7211.xz
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+c041b4ce3a6dfd1e63e2@syzkaller.appspotmail.com
>
> ==================================================================
> BUG: KASAN: slab-use-after-free in instrument_atomic_read_write include/linux/instrumented.h:96 [inline]
> BUG: KASAN: slab-use-after-free in test_and_set_bit include/asm-generic/bitops/instrumented-atomic.h:71 [inline]
> BUG: KASAN: slab-use-after-free in l2tp_session_delete+0x28/0x9e0 net/l2tp/l2tp_core.c:1639
> Write of size 8 at addr ffff88802aaf5808 by task kworker/u8:11/2523
I'll look at this.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [syzbot] [net?] KASAN: slab-use-after-free Write in l2tp_session_delete
2024-06-25 13:25 [syzbot] [net?] KASAN: slab-use-after-free Write in l2tp_session_delete syzbot
2024-06-25 14:53 ` James Chapman
@ 2024-07-02 9:52 ` syzbot
2024-07-03 9:46 ` Tom Parkin
` (3 subsequent siblings)
5 siblings, 0 replies; 15+ messages in thread
From: syzbot @ 2024-07-02 9:52 UTC (permalink / raw)
To: davem, eadavis, edumazet, hdanton, jchapman, kuba, linux-kernel,
netdev, pabeni, samuel.thibault, syzkaller-bugs, tparkin
syzbot has bisected this issue to:
commit d18d3f0a24fc4a3513495892ab1a3753628b341b
Author: James Chapman <jchapman@katalix.com>
Date: Thu Jun 20 11:22:44 2024 +0000
l2tp: replace hlist with simple list for per-tunnel session list
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=15f2a512980000
start commit: 185d72112b95 net: xilinx: axienet: Enable multicast by def..
git tree: net-next
final oops: https://syzkaller.appspot.com/x/report.txt?x=17f2a512980000
console output: https://syzkaller.appspot.com/x/log.txt?x=13f2a512980000
kernel config: https://syzkaller.appspot.com/x/.config?x=e78fc116033e0ab7
dashboard link: https://syzkaller.appspot.com/bug?extid=c041b4ce3a6dfd1e63e2
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=12521b9c980000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1062bd46980000
Reported-by: syzbot+c041b4ce3a6dfd1e63e2@syzkaller.appspotmail.com
Fixes: d18d3f0a24fc ("l2tp: replace hlist with simple list for per-tunnel session list")
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [syzbot] [net?] KASAN: slab-use-after-free Write in l2tp_session_delete
2024-06-25 13:25 [syzbot] [net?] KASAN: slab-use-after-free Write in l2tp_session_delete syzbot
2024-06-25 14:53 ` James Chapman
2024-07-02 9:52 ` syzbot
@ 2024-07-03 9:46 ` Tom Parkin
2024-07-03 10:05 ` syzbot
2024-07-03 11:24 ` Tom Parkin
` (2 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Tom Parkin @ 2024-07-03 9:46 UTC (permalink / raw)
To: syzbot; +Cc: linux-kernel, netdev, syzkaller-bugs
[-- Attachment #1: Type: text/plain, Size: 1186 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
--- 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;
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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [syzbot] [net?] KASAN: slab-use-after-free Write in l2tp_session_delete
2024-06-25 13:25 [syzbot] [net?] KASAN: slab-use-after-free Write in l2tp_session_delete syzbot
` (2 preceding siblings ...)
2024-07-03 9:46 ` Tom Parkin
@ 2024-07-03 11:24 ` Tom Parkin
2024-07-03 11:30 ` syzbot
2024-07-03 11:51 ` Hillf Danton
2024-07-03 12:07 ` Tom Parkin
2024-07-03 16:37 ` Tom Parkin
5 siblings, 2 replies; 15+ messages in thread
From: Tom Parkin @ 2024-07-03 11:24 UTC (permalink / raw)
To: syzbot; +Cc: linux-kernel, netdev, syzkaller-bugs
[-- 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;
list_del_init(&session->list);
spin_unlock_bh(&tunnel->list_lock);
l2tp_session_delete(session);
--
2.34.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [syzbot] [net?] KASAN: slab-use-after-free Write in l2tp_session_delete
2024-07-03 11:24 ` Tom Parkin
@ 2024-07-03 11:30 ` syzbot
2024-07-03 11:51 ` Hillf Danton
1 sibling, 0 replies; 15+ messages in thread
From: syzbot @ 2024-07-03 11:30 UTC (permalink / raw)
To: linux-kernel, netdev, syzkaller-bugs, tparkin
Hello,
syzbot tried to test the proposed patch but the build/boot failed:
failed to apply patch:
checking file net/l2tp/l2tp_core.c
Hunk #1 FAILED at 1290.
1 out of 1 hunk FAILED
Tested on:
commit: 185d7211 net: xilinx: axienet: Enable multicast by def..
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
kernel config: https://syzkaller.appspot.com/x/.config?x=e78fc116033e0ab7
dashboard link: https://syzkaller.appspot.com/bug?extid=c041b4ce3a6dfd1e63e2
compiler:
patch: https://syzkaller.appspot.com/x/patch.diff?x=177064e1980000
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [syzbot] [net?] KASAN: slab-use-after-free Write in l2tp_session_delete
2024-07-03 11:24 ` Tom Parkin
2024-07-03 11:30 ` syzbot
@ 2024-07-03 11:51 ` Hillf Danton
2024-07-03 12:39 ` Tom Parkin
1 sibling, 1 reply; 15+ messages in thread
From: Hillf Danton @ 2024-07-03 11:51 UTC (permalink / raw)
To: Tom Parkin; +Cc: syzbot, linux-kernel, netdev, James Chapman, syzkaller-bugs
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);
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [syzbot] [net?] KASAN: slab-use-after-free Write in l2tp_session_delete
2024-07-03 11:51 ` Hillf Danton
@ 2024-07-03 12:39 ` Tom Parkin
2024-07-04 11:23 ` Hillf Danton
0 siblings, 1 reply; 15+ messages in thread
From: Tom Parkin @ 2024-07-03 12:39 UTC (permalink / raw)
To: Hillf Danton; +Cc: syzbot, linux-kernel, netdev, James Chapman, syzkaller-bugs
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [syzbot] [net?] KASAN: slab-use-after-free Write in l2tp_session_delete
2024-07-03 12:39 ` Tom Parkin
@ 2024-07-04 11:23 ` Hillf Danton
2024-07-04 13:59 ` Tom Parkin
0 siblings, 1 reply; 15+ messages in thread
From: Hillf Danton @ 2024-07-04 11:23 UTC (permalink / raw)
To: Tom Parkin; +Cc: syzbot, linux-kernel, netdev, James Chapman, syzkaller-bugs
On Wed, 3 Jul 2024 13:39:25 +0100 Tom Parkin <tparkin@katalix.com>
>
> 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.
>
Yeah the next node could race with other thread because of the door.
> 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.
Race exists after your patch.
cpu1 cpu2
--- ---
pppol2tp_release()
spin_lock_bh(&tunnel->list_lock);
while (!list_empty(&tunnel->session_list)) {
session = list_first_entry(&tunnel->session_list,
struct l2tp_session, list);
list_del_init(&session->list);
spin_unlock_bh(&tunnel->list_lock);
l2tp_session_delete(session);
l2tp_session_delete(session);
spin_lock_bh(&tunnel->list_lock);
}
spin_unlock_bh(&tunnel->list_lock);
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [syzbot] [net?] KASAN: slab-use-after-free Write in l2tp_session_delete
2024-07-04 11:23 ` Hillf Danton
@ 2024-07-04 13:59 ` Tom Parkin
0 siblings, 0 replies; 15+ messages in thread
From: Tom Parkin @ 2024-07-04 13:59 UTC (permalink / raw)
To: Hillf Danton; +Cc: syzbot, linux-kernel, netdev, James Chapman, syzkaller-bugs
[-- Attachment #1: Type: text/plain, Size: 2379 bytes --]
On Thu, Jul 04, 2024 at 19:23:03 +0800, Hillf Danton wrote:
> On Wed, 3 Jul 2024 13:39:25 +0100 Tom Parkin <tparkin@katalix.com>
> >
> > 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.
> >
> Yeah the next node could race with other thread because of the door.
Exactly, yes.
> > 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.
>
> Race exists after your patch.
>
> cpu1 cpu2
> --- ---
> pppol2tp_release()
>
> spin_lock_bh(&tunnel->list_lock);
> while (!list_empty(&tunnel->session_list)) {
> session = list_first_entry(&tunnel->session_list,
> struct l2tp_session, list);
> list_del_init(&session->list);
> spin_unlock_bh(&tunnel->list_lock);
>
> l2tp_session_delete(session);
>
> l2tp_session_delete(session);
> spin_lock_bh(&tunnel->list_lock);
> }
> spin_unlock_bh(&tunnel->list_lock);
I take your point. Calling l2tp_session_delete() on the same session
twice isn't a problem per-se, but if cpu2 manages to destruct the
socket and unref the session to zero before cpu1 progresses then we
have the same sort of problem.
To be honest, cleanup generally could use some TLC (the dancing around
the list lock in _closeall is not ideal), but a minimal patch to
address the UAF makes sense in the short term IMO -- so to that end
holding a session reference around l2tp_session_delete in _closeall is
probably the least invasive thing to do.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [syzbot] [net?] KASAN: slab-use-after-free Write in l2tp_session_delete
2024-06-25 13:25 [syzbot] [net?] KASAN: slab-use-after-free Write in l2tp_session_delete syzbot
` (3 preceding siblings ...)
2024-07-03 11:24 ` Tom Parkin
@ 2024-07-03 12:07 ` Tom Parkin
2024-07-03 13:56 ` syzbot
2024-07-03 16:37 ` Tom Parkin
5 siblings, 1 reply; 15+ messages in thread
From: Tom Parkin @ 2024-07-03 12:07 UTC (permalink / raw)
To: syzbot; +Cc: linux-kernel, netdev, syzkaller-bugs
[-- Attachment #1.1: Type: text/plain, Size: 382 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: 3240 bytes --]
From 15d6d4c290810d5b8f357b9e494c5ad420c8a2fb Mon Sep 17 00:00:00 2001
From: Tom Parkin <tparkin@katalix.com>
Date: Wed, 3 Jul 2024 13:02:51 +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 | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index be4bcbf291a1..fa75a8eb8782 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1290,12 +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 __rcu *pos;
- struct list_head *tmp;
spin_lock_bh(&tunnel->list_lock);
tunnel->acpt_newsess = false;
- list_for_each_safe(pos, tmp, &tunnel->session_list) {
+ for (;;) {
+ session = list_first_entry_or_null(&tunnel->session_list,
+ struct l2tp_session, list);
+ if (!session)
+ break;
session = list_entry(pos, struct l2tp_session, list);
list_del_init(&session->list);
spin_unlock_bh(&tunnel->list_lock);
--
2.34.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [syzbot] [net?] KASAN: slab-use-after-free Write in l2tp_session_delete
2024-06-25 13:25 [syzbot] [net?] KASAN: slab-use-after-free Write in l2tp_session_delete syzbot
` (4 preceding siblings ...)
2024-07-03 12:07 ` Tom Parkin
@ 2024-07-03 16:37 ` Tom Parkin
2024-07-03 17:23 ` syzbot
5 siblings, 1 reply; 15+ messages in thread
From: Tom Parkin @ 2024-07-03 16:37 UTC (permalink / raw)
To: syzbot; +Cc: linux-kernel, netdev, syzkaller-bugs
[-- 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 6bc347af14eba3b37f9e8f0831a53ef1aae4c203 Mon Sep 17 00:00:00 2001
From: Tom Parkin <tparkin@katalix.com>
Date: Wed, 3 Jul 2024 17:32:00 +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 be4bcbf291a1..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 __rcu *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;
list_del_init(&session->list);
spin_unlock_bh(&tunnel->list_lock);
l2tp_session_delete(session);
--
2.34.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply related [flat|nested] 15+ messages in thread