[PATCH net] l2tp: add sk_reuseport checks to l2tp_validate_socket

[PATCH net] l2tp: add sk_reuseport checks to l2tp_validate_socket

[James Chapman]

syzbot is able to trigger a BUG_ON in l2tp by setting SO_REUSEPORT on
a UDP socket which is then used by l2tp. However, the bug occurs only
if the kernel has CONFIG_BPF_SYSCALL enabled.

kernel BUG at net/l2tp/l2tp_core.c:1572!
invalid opcode: 0000 [#1] PREEMPT SMP KASAN
CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.7.0-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
RIP: 0010:l2tp_session_free+0x1ee/0x1f0 net/l2tp/l2tp_core.c:1572
Code: 44 89 e1 80 e1 07 80 c1 03 38 c1 0f 8c b8 fe ff ff 4c 89 e7 e8 03 64 37 fa e9 ab fe ff ff e8 d9 7f f8 f9 0f 0b e8 d2 7f f8 f9 <0f> 0b 55 41 57 41 56 41 55 41 54 53 49 89 fe 48 bd 00 00 00 00 00
RSP: 0018:ffffc90000da8da8 EFLAGS: 00010246
RAX: ffffffff877c235e RBX: 0000000000000000 RCX: ffff8880a99f4340
RDX: 0000000080000101 RSI: 0000000000000000 RDI: 0000000042114dda
RBP: ffff8880a7958238 R08: ffffffff877c220d R09: ffffed1014f2b01a
R10: ffffed1014f2b01a R11: 0000000000000000 R12: ffff8880a6f77800
R13: dffffc0000000000 R14: ffff8880a7958000 R15: dffffc0000000000
FS:  0000000000000000(0000) GS:ffff8880ae900000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000000049f410 CR3: 0000000009279000 CR4: 00000000001406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 <IRQ>
 __sk_destruct+0x50/0x740 net/core/sock.c:1785
 rcu_do_batch kernel/rcu/tree.c:2396 [inline]
 rcu_core+0x90c/0x1200 kernel/rcu/tree.c:2623
 __do_softirq+0x268/0x80c kernel/softirq.c:292
 invoke_softirq kernel/softirq.c:373 [inline]
 irq_exit+0x223/0x230 kernel/softirq.c:413
 exiting_irq arch/x86/include/asm/apic.h:546 [inline]
 smp_apic_timer_interrupt+0x113/0x280 arch/x86/kernel/apic/apic.c:1107
 apic_timer_interrupt+0xf/0x20 arch/x86/entry/entry_64.S:829
 </IRQ>
RIP: 0010:native_safe_halt+0xe/0x10 arch/x86/include/asm/irqflags.h:61
Code: 80 e1 07 80 c1 03 38 c1 7c bc 48 89 df e8 9a 17 9b f9 eb b2 cc cc cc cc cc cc cc cc e9 07 00 00 00 0f 00 2d 56 46 4a 00 fb f4 <c3> 90 e9 07 00 00 00 0f 00 2d 46 46 4a 00 f4 c3 cc cc 41 56 53 65
RSP: 0018:ffffc90000d3fd60 EFLAGS: 00000286 ORIG_RAX: ffffffffffffff13
RAX: 1ffffffff12577b9 RBX: 0000000000000000 RCX: ffffffffffffffff
RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff8880a99f4ba4
RBP: 1ffff1104351244e R08: ffffffff817a4660 R09: ffffed101533e869
R10: ffffed101533e869 R11: 0000000000000000 R12: 0000000000000001
R13: dffffc0000000000 R14: dffffc0000000000 R15: ffff88821a892270
 arch_safe_halt arch/x86/include/asm/paravirt.h:150 [inline]
 acpi_safe_halt+0x87/0xe0 drivers/acpi/processor_idle.c:111
 acpi_idle_do_entry drivers/acpi/processor_idle.c:525 [inline]
 acpi_idle_enter+0x3f4/0xac0 drivers/acpi/processor_idle.c:651
 cpuidle_enter_state+0x2d7/0x7b0 drivers/cpuidle/cpuidle.c:234
 cpuidle_enter+0x59/0x90 drivers/cpuidle/cpuidle.c:345
 call_cpuidle kernel/sched/idle.c:117 [inline]
 cpuidle_idle_call kernel/sched/idle.c:207 [inline]
 do_idle+0x49c/0x650 kernel/sched/idle.c:269
 cpu_startup_entry+0x15/0x20 kernel/sched/idle.c:365
 start_secondary+0x213/0x240 arch/x86/kernel/smpboot.c:268
 secondary_startup_64+0xa4/0xb0 arch/x86/kernel/head_64.S:242
Modules linked in:
---[ end trace 1d65b89c4fe927df ]---
RIP: 0010:l2tp_session_free+0x1ee/0x1f0 net/l2tp/l2tp_core.c:1572
Code: 44 89 e1 80 e1 07 80 c1 03 38 c1 0f 8c b8 fe ff ff 4c 89 e7 e8 03 64 37 fa e9 ab fe ff ff e8 d9 7f f8 f9 0f 0b e8 d2 7f f8 f9 <0f> 0b 55 41 57 41 56 41 55 41 54 53 49 89 fe 48 bd 00 00 00 00 00
RSP: 0018:ffffc90000da8da8 EFLAGS: 00010246
RAX: ffffffff877c235e RBX: 0000000000000000 RCX: ffff8880a99f4340
RDX: 0000000080000101 RSI: 0000000000000000 RDI: 0000000042114dda
RBP: ffff8880a7958238 R08: ffffffff877c220d R09: ffffed1014f2b01a
R10: ffffed1014f2b01a R11: 0000000000000000 R12: ffff8880a6f77800
R13: dffffc0000000000 R14: ffff8880a7958000 R15: dffffc0000000000
FS:  0000000000000000(0000) GS:ffff8880ae900000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000000049f410 CR3: 0000000009279000 CR4: 00000000001406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400

The bug is triggered by a simple sequence:

 [pid 21579] socket(AF_PPPOX, SOCK_STREAM, 1) = 3
 [pid 21579] socket(AF_INET6, SOCK_DGRAM|SOCK_CLOEXEC, IPPROTO_IP) = 4
 [pid 21579] setsockopt(4, SOL_SOCKET, SO_REUSEPORT, [11], 4) = 0
 [pid 21579] bind(4, {sa_family=AF_INET6, sin6_port=htons(20000), sin6_flowinfo=htonl(0), inet_pton(AF_INET6, "::", &sin6_addr), sin6_scope_id=0}, 28) = 0
 [pid 21579] connect(3, {sa_family=AF_PPPOX, sa_data="\1\0\0\0\0\0\0\0\4\0\0\0\1\0\0\0\0\0\0\0\1\0\0\0\0\0\0\0\n\0N \0\0\0\7\0\0\0\0\0\0\0\0\0\0\377\377\254\24\24\22\377\3\0\0"}, 58) = 0
 [pid 21579] close(3)                    = 0
 [pid 21579] close(4)                    = 0

The crash occurs in the socket destroy path. bpf_sk_reuseport_detach
assumes ownership of sk_user_data if sk_reuseport is set and writes a
NULL pointer to the memory pointed to by
sk_user_data. bpf_sk_reuseport_detach is called via
udp_lib_unhash. l2tp does its socket cleanup through sk_destruct,
which fetches private data through sk_user_data. The BUG_ON fires
because this data has been corrupted.

Prevent this crash by adding a check for sk_reuseport in
l2tp_validate_socket. l2tp won't be able to use SO_REUSEPORT.

This brings up two questions:

 1. If CONFIG_BPF_SYSCALL is enabled, should the SO_REUSEPORT setsockopt
    handler check that sk_user_data isn't already set on the socket?
    This crash could also occur if SO_REUSEPORT were set after the
    socket is initialised by l2tp.

 2. Should the reuseport code have a dedicated member of struct sock
    to use instead of sk_user_data such that SO_REUSEPORT can be used
    by UDP encap socket users like l2tp?

Fixes: 6b9f34239b00 ("l2tp: fix races in tunnel creation")
Signed-off-by: James Chapman <jchapman@katalix.com>
Cc: Guillaume Nault <gnault@redhat.com>
Reported-by: syzbot <syzkaller@googlegroups.com>
---
 net/l2tp/l2tp_core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 6d7ef78c88af..5bfd046326ca 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1465,6 +1465,9 @@ static int l2tp_validate_socket(const struct sock *sk, const struct net *net,
 	    (encap == L2TP_ENCAPTYPE_IP && sk->sk_protocol != IPPROTO_L2TP))
 		return -EPROTONOSUPPORT;
 
+	if (sk->sk_reuseport)
+		return -EPROTONOSUPPORT;
+
 	if (sk->sk_user_data)
 		return -EBUSY;
 
-- 
2.17.1

Re: [PATCH net] l2tp: add sk_reuseport checks to l2tp_validate_socket

[David Miller]

From: James Chapman <jchapman@katalix.com>
Date: Mon, 6 Jul 2020 13:12:59 +0100

> The crash occurs in the socket destroy path. bpf_sk_reuseport_detach
> assumes ownership of sk_user_data if sk_reuseport is set and writes a
> NULL pointer to the memory pointed to by
> sk_user_data. bpf_sk_reuseport_detach is called via
> udp_lib_unhash. l2tp does its socket cleanup through sk_destruct,
> which fetches private data through sk_user_data. The BUG_ON fires
> because this data has been corrupted.

The ownership of sk_user_data has to be handled more cleanly.

BPF really has no business taking over this as it is for the protocols
to use and what L2TP is doing is quite natural and normal.  Exactly
what sk_user_data was designed to be used for.

I'm not applying this, please take this up with the BPF folks.  They
need to store their metadata elsewhere.

Re: [PATCH net] l2tp: add sk_reuseport checks to l2tp_validate_socket

[Martin KaFai Lau]

On Mon, Jul 06, 2020 at 12:45:36PM -0700, David Miller wrote:
> From: James Chapman <jchapman@katalix.com>
> Date: Mon, 6 Jul 2020 13:12:59 +0100
> 
> > The crash occurs in the socket destroy path. bpf_sk_reuseport_detach
> > assumes ownership of sk_user_data if sk_reuseport is set and writes a
> > NULL pointer to the memory pointed to by
> > sk_user_data. bpf_sk_reuseport_detach is called via
> > udp_lib_unhash. l2tp does its socket cleanup through sk_destruct,
> > which fetches private data through sk_user_data. The BUG_ON fires
> > because this data has been corrupted.
> 
> The ownership of sk_user_data has to be handled more cleanly.
> 
> BPF really has no business taking over this as it is for the protocols
> to use and what L2TP is doing is quite natural and normal.  Exactly
> what sk_user_data was designed to be used for.
> 
> I'm not applying this, please take this up with the BPF folks.  They
> need to store their metadata elsewhere.
Thanks for the report.

The sk_user_data is used when a sk is added to the bpf's reuseport_sockarray.
Before it can be added, the bpf side does check if the sk_user_data has already been
used or not.  It is the similar check like other usages on sk_user_data.

The missing part is the reuseport_detach_sock() should check if a
sk is currently in a reuseport_sockarray before calling bpf_sk_reuseport_detach().
It can be solved by remembering if a sk is added to the reuseport_sockarray.
I will work on a fix by doing this.

Re: [PATCH net] l2tp: add sk_reuseport checks to l2tp_validate_socket

[James Chapman]

On  Tue, Jul 07, 2020 at 11:31:28 -0700, Martin KaFai Lau wrote:
> On Mon, Jul 06, 2020 at 12:45:36PM -0700, David Miller wrote:
> > From: James Chapman <jchapman@katalix.com>
> > Date: Mon, 6 Jul 2020 13:12:59 +0100
> > 
> > > The crash occurs in the socket destroy path. bpf_sk_reuseport_detach
> > > assumes ownership of sk_user_data if sk_reuseport is set and writes a
> > > NULL pointer to the memory pointed to by
> > > sk_user_data. bpf_sk_reuseport_detach is called via
> > > udp_lib_unhash. l2tp does its socket cleanup through sk_destruct,
> > > which fetches private data through sk_user_data. The BUG_ON fires
> > > because this data has been corrupted.
> > 
> > The ownership of sk_user_data has to be handled more cleanly.
> > 
> > BPF really has no business taking over this as it is for the protocols
> > to use and what L2TP is doing is quite natural and normal.  Exactly
> > what sk_user_data was designed to be used for.
> > 
> > I'm not applying this, please take this up with the BPF folks.  They
> > need to store their metadata elsewhere.
> Thanks for the report.
> 
> The sk_user_data is used when a sk is added to the bpf's reuseport_sockarray.
> Before it can be added, the bpf side does check if the sk_user_data has already been
> used or not.  It is the similar check like other usages on sk_user_data.
> 
> The missing part is the reuseport_detach_sock() should check if a
> sk is currently in a reuseport_sockarray before calling bpf_sk_reuseport_detach().
> It can be solved by remembering if a sk is added to the reuseport_sockarray.
> I will work on a fix by doing this.

Thanks Martin.