diff mbox series

[net-next,v2,02/16] l2tp: add RCU read lock to protect tunnel ptr in ip socket destroy

Message ID 1518430280-16671-3-git-send-email-jchapman@katalix.com
State Changes Requested, archived
Delegated to: David Miller
Headers show
Series l2tp: fix API races discovered by syzbot | expand

Commit Message

James Chapman Feb. 12, 2018, 10:11 a.m. UTC 
If an L2TPIP socket is closed, add RCU protection when we deref
sk_user_data to prevent races with another thread closing the same
tunnel.

Fixes: 0d76751fad ("l2tp: Add L2TPv3 IP encapsulation (no UDP) support")

 refcount_t: increment on 0; use-after-free.
 WARNING: CPU: 2 PID: 2892 at lib/refcount.c:153 refcount_inc+0x2b/0x30
 Modules linked in:
 CPU: 2 PID: 2892 Comm: pppol2tp_chaos Not tainted 4.15.0-rc9+ #1
 Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
 RIP: 0010:refcount_inc+0x2b/0x30
 RSP: 0018:ffff880014147b50 EFLAGS: 00010282
 RAX: 000000000000002b RBX: ffff8800194785c0 RCX: 0000000000000000
 RDX: 0000000000000001 RSI: ffff88001ad1f758 RDI: ffff88001ad1f758
 RBP: ffff880014147b50 R08: 0000000000000000 R09: 0000000000000000
 R10: 0000000000000001 R11: 0000000000000001 R12: ffff8800194785c8
 R13: ffff8800194785c0 R14: ffff88001a2c6c20 R15: ffff880013a9d580
 FS:  0000000000000000(0000) GS:ffff88001ad00000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 00007fbc17ea7000 CR3: 0000000003022000 CR4: 00000000000006e0
 Call Trace:
  l2tp_tunnel_delete+0x34/0x60
  l2tp_ip_destroy_sock+0x6d/0x80
  sk_common_release+0x19/0xd0
  l2tp_ip_close+0x89/0xa0
  inet_release+0x37/0x60
  sock_release+0x1a/0x70
  sock_close+0xd/0x20
  __fput+0xed/0x1f0
  ____fput+0x9/0x10
  task_work_run+0x77/0xb0
  do_exit+0x311/0xcf0
  do_group_exit+0x47/0xc0
  get_signal+0x343/0x8e0
  do_signal+0x23/0x780
  ? find_held_lock+0x39/0xb0
  exit_to_usermode_loop+0x4a/0x93
  syscall_return_slowpath+0x102/0x150
  entry_SYSCALL_64_fastpath+0x98/0x9a
 RIP: 0033:0x7fbc172d7fcf
 RSP: 002b:00007ffd524cd4d8 EFLAGS: 00000246 ORIG_RAX: 000000000000000e
 RAX: 0000000000000000 RBX: 0000000000000006 RCX: 00007fbc172d7fcf
 RDX: 0000000000000000 RSI: 00007ffd524cd460 RDI: 0000000000000002
 RBP: 0000000000404930 R08: 0000000000000000 R09: 00007ffd524cd460
 R10: 0000000000000008 R11: 0000000000000246 R12: 000000000000001e
 R13: 0000000000404a03 R14: 0000000000000000 R15: 0000000000000000
 Code: 55 48 89 e5 e8 87 ff ff ff 84 c0 74 02 5d c3 80 3d 97 87 bb 01 00 75 f5 48 c7 c7 58 3e cc 82 c6 05 87 87 bb 01
---
 net/l2tp/l2tp_ip.c  | 5 ++++-
 net/l2tp/l2tp_ip6.c | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

Comments

David Miller Feb. 12, 2018, 4:22 p.m. UTC | #1 
From: James Chapman <jchapman@katalix.com>
Date: Mon, 12 Feb 2018 10:11:06 +0000

> If an L2TPIP socket is closed, add RCU protection when we deref
> sk_user_data to prevent races with another thread closing the same
> tunnel.
> 
> Fixes: 0d76751fad ("l2tp: Add L2TPv3 IP encapsulation (no UDP) support")

Another patch with no signoff, and:

>  refcount_t: increment on 0; use-after-free.
>  WARNING: CPU: 2 PID: 2892 at lib/refcount.c:153 refcount_inc+0x2b/0x30
>  Modules linked in:
>  CPU: 2 PID: 2892 Comm: pppol2tp_chaos Not tainted 4.15.0-rc9+ #1
 ...

a kernel log dump which should be in the commit message body.
Guillaume Nault Feb. 12, 2018, 6:35 p.m. UTC | #2 
On Mon, Feb 12, 2018 at 10:11:06AM +0000, James Chapman wrote:
> If an L2TPIP socket is closed, add RCU protection when we deref
> sk_user_data to prevent races with another thread closing the same
> tunnel.
> 
> Fixes: 0d76751fad ("l2tp: Add L2TPv3 IP encapsulation (no UDP) support")
> 
>  refcount_t: increment on 0; use-after-free.
>  WARNING: CPU: 2 PID: 2892 at lib/refcount.c:153 refcount_inc+0x2b/0x30
>  Modules linked in:
>  CPU: 2 PID: 2892 Comm: pppol2tp_chaos Not tainted 4.15.0-rc9+ #1
>  Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
>  RIP: 0010:refcount_inc+0x2b/0x30
>  RSP: 0018:ffff880014147b50 EFLAGS: 00010282
>  RAX: 000000000000002b RBX: ffff8800194785c0 RCX: 0000000000000000
>  RDX: 0000000000000001 RSI: ffff88001ad1f758 RDI: ffff88001ad1f758
>  RBP: ffff880014147b50 R08: 0000000000000000 R09: 0000000000000000
>  R10: 0000000000000001 R11: 0000000000000001 R12: ffff8800194785c8
>  R13: ffff8800194785c0 R14: ffff88001a2c6c20 R15: ffff880013a9d580
>  FS:  0000000000000000(0000) GS:ffff88001ad00000(0000) knlGS:0000000000000000
>  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>  CR2: 00007fbc17ea7000 CR3: 0000000003022000 CR4: 00000000000006e0
>  Call Trace:
>   l2tp_tunnel_delete+0x34/0x60
>   l2tp_ip_destroy_sock+0x6d/0x80
>   sk_common_release+0x19/0xd0
>   l2tp_ip_close+0x89/0xa0
>   inet_release+0x37/0x60
>   sock_release+0x1a/0x70
>   sock_close+0xd/0x20
>   __fput+0xed/0x1f0
>   ____fput+0x9/0x10
>   task_work_run+0x77/0xb0
>   do_exit+0x311/0xcf0
>   do_group_exit+0x47/0xc0
>   get_signal+0x343/0x8e0
>   do_signal+0x23/0x780
>   ? find_held_lock+0x39/0xb0
>   exit_to_usermode_loop+0x4a/0x93
>   syscall_return_slowpath+0x102/0x150
>   entry_SYSCALL_64_fastpath+0x98/0x9a
>  RIP: 0033:0x7fbc172d7fcf
>  RSP: 002b:00007ffd524cd4d8 EFLAGS: 00000246 ORIG_RAX: 000000000000000e
>  RAX: 0000000000000000 RBX: 0000000000000006 RCX: 00007fbc172d7fcf
>  RDX: 0000000000000000 RSI: 00007ffd524cd460 RDI: 0000000000000002
>  RBP: 0000000000404930 R08: 0000000000000000 R09: 00007ffd524cd460
>  R10: 0000000000000008 R11: 0000000000000246 R12: 000000000000001e
>  R13: 0000000000404a03 R14: 0000000000000000 R15: 0000000000000000
>  Code: 55 48 89 e5 e8 87 ff ff ff 84 c0 74 02 5d c3 80 3d 97 87 bb 01 00 75 f5 48 c7 c7 58 3e cc 82 c6 05 87 87 bb 01
> ---
>  net/l2tp/l2tp_ip.c  | 5 ++++-
>  net/l2tp/l2tp_ip6.c | 5 ++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c
> index ff61124fdf59..42f3c2f72bf4 100644
> --- a/net/l2tp/l2tp_ip.c
> +++ b/net/l2tp/l2tp_ip.c
> @@ -234,15 +234,18 @@ static void l2tp_ip_close(struct sock *sk, long timeout)
>  static void l2tp_ip_destroy_sock(struct sock *sk)
>  {
>  	struct sk_buff *skb;
> -	struct l2tp_tunnel *tunnel = l2tp_sock_to_tunnel(sk);
> +	struct l2tp_tunnel *tunnel;
>  
>  	while ((skb = __skb_dequeue_tail(&sk->sk_write_queue)) != NULL)
>  		kfree_skb(skb);
>  
> +	rcu_read_lock();
> +	tunnel = rcu_dereference_sk_user_data(sk);
Is this safe? We don't wait for a grace period after resetting ->sk_user_data.

>  	if (tunnel) {
>  		l2tp_tunnel_closeall(tunnel);
I'm pretty sure l2tp_tunnel_closeall() can sleep and can't be called in
RCU critical sections.

>  		sock_put(sk);
>  	}
> +	rcu_read_unlock();
>
diff mbox series

Patch

diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c
index ff61124fdf59..42f3c2f72bf4 100644
--- a/net/l2tp/l2tp_ip.c
+++ b/net/l2tp/l2tp_ip.c
@@ -234,15 +234,18 @@  static void l2tp_ip_close(struct sock *sk, long timeout)
 static void l2tp_ip_destroy_sock(struct sock *sk)
 {
 	struct sk_buff *skb;
-	struct l2tp_tunnel *tunnel = l2tp_sock_to_tunnel(sk);
+	struct l2tp_tunnel *tunnel;
 
 	while ((skb = __skb_dequeue_tail(&sk->sk_write_queue)) != NULL)
 		kfree_skb(skb);
 
+	rcu_read_lock();
+	tunnel = rcu_dereference_sk_user_data(sk);
 	if (tunnel) {
 		l2tp_tunnel_closeall(tunnel);
 		sock_put(sk);
 	}
+	rcu_read_unlock();
 
 	sk_refcnt_debug_dec(sk);
 }
diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c
index 192344688c06..be4a3eee85a9 100644
--- a/net/l2tp/l2tp_ip6.c
+++ b/net/l2tp/l2tp_ip6.c
@@ -248,16 +248,19 @@  static void l2tp_ip6_close(struct sock *sk, long timeout)
 
 static void l2tp_ip6_destroy_sock(struct sock *sk)
 {
-	struct l2tp_tunnel *tunnel = l2tp_sock_to_tunnel(sk);
+	struct l2tp_tunnel *tunnel;
 
 	lock_sock(sk);
 	ip6_flush_pending_frames(sk);
 	release_sock(sk);
 
+	rcu_read_lock();
+	tunnel = rcu_dereference_sk_user_data(sk);
 	if (tunnel) {
 		l2tp_tunnel_closeall(tunnel);
 		sock_put(sk);
 	}
+	rcu_read_unlock();
 
 	inet6_destroy_sock(sk);
 }