From 76a6abdb2513ad4ea0ded55d2c66160491f2e848 Mon Sep 17 00:00:00 2001 From: James Chapman Date: Fri, 23 Feb 2018 17:45:43 +0000 Subject: l2tp: don't use inet_shutdown on tunnel destroy Previously, if a tunnel was closed, we called inet_shutdown to mark the socket as unconnected such that userspace would get errors and then close the socket. This could race with userspace closing the socket. Instead, leave userspace to close the socket in its own time (our tunnel will be detached anyway). BUG: unable to handle kernel NULL pointer dereference at 00000000000000a0 IP: __lock_acquire+0x263/0x1630 PGD 0 P4D 0 Oops: 0000 [#1] SMP KASAN Modules linked in: CPU: 2 PID: 42 Comm: kworker/u8:2 Not tainted 4.15.0-rc7+ #129 Workqueue: l2tp l2tp_tunnel_del_work RIP: 0010:__lock_acquire+0x263/0x1630 RSP: 0018:ffff88001a37fc70 EFLAGS: 00010002 RAX: 0000000000000001 RBX: 0000000000000088 RCX: 0000000000000000 RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000 RBP: ffff88001a37fd18 R08: 0000000000000001 R09: 0000000000000000 R10: 0000000000000000 R11: 00000000000076fd R12: 00000000000000a0 R13: ffff88001a3722c0 R14: 0000000000000001 R15: 0000000000000000 FS: 0000000000000000(0000) GS:ffff88001ad00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00000000000000a0 CR3: 000000001730b000 CR4: 00000000000006e0 Call Trace: ? __lock_acquire+0xc77/0x1630 ? console_trylock+0x11/0xa0 lock_acquire+0x117/0x230 ? lock_sock_nested+0x3a/0xa0 _raw_spin_lock_bh+0x3a/0x50 ? lock_sock_nested+0x3a/0xa0 lock_sock_nested+0x3a/0xa0 inet_shutdown+0x33/0xf0 l2tp_tunnel_del_work+0x60/0xef process_one_work+0x1ea/0x5f0 ? process_one_work+0x162/0x5f0 worker_thread+0x48/0x3e0 ? trace_hardirqs_on+0xd/0x10 kthread+0x108/0x140 ? process_one_work+0x5f0/0x5f0 ? kthread_stop+0x2a0/0x2a0 ret_from_fork+0x24/0x30 Code: 00 41 81 ff ff 1f 00 00 0f 87 7a 13 00 00 45 85 f6 49 8b 85 68 08 00 00 0f 84 ae 03 00 00 c7 44 24 18 00 00 00 00 e9 f0 00 00 00 <49> 81 3c 24 80 93 3f 83 b8 00 00 00 00 44 0f 44 c0 83 fe 01 0f RIP: __lock_acquire+0x263/0x1630 RSP: ffff88001a37fc70 CR2: 00000000000000a0 Fixes: 309795f4bec2d ("l2tp: Add netlink control API for L2TP") Signed-off-by: James Chapman Signed-off-by: David S. Miller --- net/l2tp/l2tp_core.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) (limited to 'net/l2tp') diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index 194a7483bb93..9cd2a99d0752 100644 --- a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c @@ -1327,17 +1327,10 @@ static void l2tp_tunnel_del_work(struct work_struct *work) sock = sk->sk_socket; - /* If the tunnel socket was created by userspace, then go through the - * inet layer to shut the socket down, and let userspace close it. - * Otherwise, if we created the socket directly within the kernel, use + /* If the tunnel socket was created within the kernel, use * the sk API to release it here. - * In either case the tunnel resources are freed in the socket - * destructor when the tunnel socket goes away. */ - if (tunnel->fd >= 0) { - if (sock) - inet_shutdown(sock, 2); - } else { + if (tunnel->fd < 0) { if (sock) { kernel_sock_shutdown(sock, SHUT_RDWR); sock_release(sock); -- cgit v1.2.3-59-g8ed1b From 225eb26489d05c679a4c4197ffcb81c81e9dcaf4 Mon Sep 17 00:00:00 2001 From: James Chapman Date: Fri, 23 Feb 2018 17:45:44 +0000 Subject: l2tp: don't use inet_shutdown on ppp session destroy Previously, if a ppp session was closed, we called inet_shutdown to mark the socket as unconnected such that userspace would get errors and then close the socket. This could race with userspace closing the socket. Instead, leave userspace to close the socket in its own time (our session will be detached anyway). BUG: KASAN: use-after-free in inet_shutdown+0x5d/0x1c0 Read of size 4 at addr ffff880010ea3ac0 by task syzbot_347bd5ac/8296 CPU: 3 PID: 8296 Comm: syzbot_347bd5ac Not tainted 4.16.0-rc1+ #91 Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006 Call Trace: dump_stack+0x101/0x157 ? inet_shutdown+0x5d/0x1c0 print_address_description+0x78/0x260 ? inet_shutdown+0x5d/0x1c0 kasan_report+0x240/0x360 __asan_load4+0x78/0x80 inet_shutdown+0x5d/0x1c0 ? pppol2tp_show+0x80/0x80 pppol2tp_session_close+0x68/0xb0 l2tp_tunnel_closeall+0x199/0x210 ? udp_v6_flush_pending_frames+0x90/0x90 l2tp_udp_encap_destroy+0x6b/0xc0 ? l2tp_tunnel_del_work+0x2e0/0x2e0 udpv6_destroy_sock+0x8c/0x90 sk_common_release+0x47/0x190 udp_lib_close+0x15/0x20 inet_release+0x85/0xd0 inet6_release+0x43/0x60 sock_release+0x53/0x100 ? sock_alloc_file+0x260/0x260 sock_close+0x1b/0x20 __fput+0x19f/0x380 ____fput+0x1a/0x20 task_work_run+0xd2/0x110 exit_to_usermode_loop+0x18d/0x190 do_syscall_64+0x389/0x3b0 entry_SYSCALL_64_after_hwframe+0x26/0x9b RIP: 0033:0x7fe240a45259 RSP: 002b:00007fe241132df8 EFLAGS: 00000297 ORIG_RAX: 0000000000000003 RAX: 0000000000000000 RBX: 0000000000000000 RCX: 00007fe240a45259 RDX: 00007fe240a45259 RSI: 0000000000000000 RDI: 00000000000000a5 RBP: 00007fe241132e20 R08: 00007fe241133700 R09: 0000000000000000 R10: 00007fe241133700 R11: 0000000000000297 R12: 0000000000000000 R13: 00007ffc49aff84f R14: 0000000000000000 R15: 00007fe241141040 Allocated by task 8331: save_stack+0x43/0xd0 kasan_kmalloc+0xad/0xe0 kasan_slab_alloc+0x12/0x20 kmem_cache_alloc+0x144/0x3e0 sock_alloc_inode+0x22/0x130 alloc_inode+0x3d/0xf0 new_inode_pseudo+0x1c/0x90 sock_alloc+0x30/0x110 __sock_create+0xaa/0x4c0 SyS_socket+0xbe/0x130 do_syscall_64+0x128/0x3b0 entry_SYSCALL_64_after_hwframe+0x26/0x9b Freed by task 8314: save_stack+0x43/0xd0 __kasan_slab_free+0x11a/0x170 kasan_slab_free+0xe/0x10 kmem_cache_free+0x88/0x2b0 sock_destroy_inode+0x49/0x50 destroy_inode+0x77/0xb0 evict+0x285/0x340 iput+0x429/0x530 dentry_unlink_inode+0x28c/0x2c0 __dentry_kill+0x1e3/0x2f0 dput.part.21+0x500/0x560 dput+0x24/0x30 __fput+0x2aa/0x380 ____fput+0x1a/0x20 task_work_run+0xd2/0x110 exit_to_usermode_loop+0x18d/0x190 do_syscall_64+0x389/0x3b0 entry_SYSCALL_64_after_hwframe+0x26/0x9b Fixes: fd558d186df2c ("l2tp: Split pppol2tp patch into separate l2tp and ppp parts") Signed-off-by: James Chapman Signed-off-by: David S. Miller --- net/l2tp/l2tp_ppp.c | 10 ---------- 1 file changed, 10 deletions(-) (limited to 'net/l2tp') diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c index 59f246d7b290..2d2955e8f710 100644 --- a/net/l2tp/l2tp_ppp.c +++ b/net/l2tp/l2tp_ppp.c @@ -420,16 +420,6 @@ abort: */ static void pppol2tp_session_close(struct l2tp_session *session) { - struct sock *sk; - - BUG_ON(session->magic != L2TP_SESSION_MAGIC); - - sk = pppol2tp_session_get_sock(session); - if (sk) { - if (sk->sk_socket) - inet_shutdown(sk->sk_socket, SEND_SHUTDOWN); - sock_put(sk); - } } /* Really kill the session socket. (Called from sock_put() if -- cgit v1.2.3-59-g8ed1b From d00fa9adc528c1b0e64d532556764852df8bd7b9 Mon Sep 17 00:00:00 2001 From: James Chapman Date: Fri, 23 Feb 2018 17:45:45 +0000 Subject: l2tp: fix races with tunnel socket close The tunnel socket tunnel->sock (struct sock) is accessed when preparing a new ppp session on a tunnel at pppol2tp_session_init. If the socket is closed by a thread while another is creating a new session, the threads race. In pppol2tp_connect, the tunnel object may be created if the pppol2tp socket is associated with the special session_id 0 and the tunnel socket is looked up using the provided fd. When handling this, pppol2tp_connect cannot sock_hold the tunnel socket to prevent it being destroyed during pppol2tp_connect since this may itself may race with the socket being destroyed. Doing sockfd_lookup in pppol2tp_connect isn't sufficient to prevent tunnel->sock going away either because a given tunnel socket fd may be reused between calls to pppol2tp_connect. Instead, have l2tp_tunnel_create sock_hold the tunnel socket before it does sockfd_put. This ensures that the tunnel's socket is always extant while the tunnel object exists. Hold a ref on the socket until the tunnel is destroyed and ensure that all tunnel destroy paths go through a common function (l2tp_tunnel_delete) since this will do the final sock_put to release the tunnel socket. Since the tunnel's socket is now guaranteed to exist if the tunnel exists, we no longer need to use sockfd_lookup via l2tp_sock_to_tunnel to derive the tunnel from the socket since this is always sk_user_data. Also, sessions no longer sock_hold the tunnel socket since sessions already hold a tunnel ref and the tunnel sock will not be freed until the tunnel is freed. Removing these sock_holds in l2tp_session_register avoids a possible sock leak in the pppol2tp_connect error path if l2tp_session_register succeeds but attaching a ppp channel fails. The pppol2tp_connect error path could have been fixed instead and have the sock ref dropped when the session is freed, but doing a sock_put of the tunnel socket when the session is freed would require a new session_free callback. It is simpler to just remove the sock_hold of the tunnel socket in l2tp_session_register, now that the tunnel socket lifetime is guaranteed. Finally, some init code in l2tp_tunnel_create is reordered to ensure that the new tunnel object's refcount is set and the tunnel socket ref is taken before the tunnel socket destructor callbacks are set. kasan: CONFIG_KASAN_INLINE enabled kasan: GPF could be caused by NULL-ptr deref or user memory access general protection fault: 0000 [#1] SMP KASAN Modules linked in: CPU: 0 PID: 4360 Comm: syzbot_19c09769 Not tainted 4.16.0-rc2+ #34 Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006 RIP: 0010:pppol2tp_session_init+0x1d6/0x500 RSP: 0018:ffff88001377fb40 EFLAGS: 00010212 RAX: dffffc0000000000 RBX: ffff88001636a940 RCX: ffffffff84836c1d RDX: 0000000000000045 RSI: 0000000055976744 RDI: 0000000000000228 RBP: ffff88001377fb60 R08: ffffffff84836bc8 R09: 0000000000000002 R10: ffff88001377fab8 R11: 0000000000000001 R12: 0000000000000000 R13: ffff88001636aac8 R14: ffff8800160f81c0 R15: 1ffff100026eff76 FS: 00007ffb3ea66700(0000) GS:ffff88001a400000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000020e77000 CR3: 0000000016261000 CR4: 00000000000006f0 Call Trace: pppol2tp_connect+0xd18/0x13c0 ? pppol2tp_session_create+0x170/0x170 ? __might_fault+0x115/0x1d0 ? lock_downgrade+0x860/0x860 ? __might_fault+0xe5/0x1d0 ? security_socket_connect+0x8e/0xc0 SYSC_connect+0x1b6/0x310 ? SYSC_bind+0x280/0x280 ? __do_page_fault+0x5d1/0xca0 ? up_read+0x1f/0x40 ? __do_page_fault+0x3c8/0xca0 SyS_connect+0x29/0x30 ? SyS_accept+0x40/0x40 do_syscall_64+0x1e0/0x730 ? trace_hardirqs_off_thunk+0x1a/0x1c entry_SYSCALL_64_after_hwframe+0x42/0xb7 RIP: 0033:0x7ffb3e376259 RSP: 002b:00007ffeda4f6508 EFLAGS: 00000202 ORIG_RAX: 000000000000002a RAX: ffffffffffffffda RBX: 0000000020e77012 RCX: 00007ffb3e376259 RDX: 000000000000002e RSI: 0000000020e77000 RDI: 0000000000000004 RBP: 00007ffeda4f6540 R08: 0000000000000000 R09: 0000000000000000 R10: 0000000000000000 R11: 0000000000000202 R12: 0000000000400b60 R13: 00007ffeda4f6660 R14: 0000000000000000 R15: 0000000000000000 Code: 80 3d b0 ff 06 02 00 0f 84 07 02 00 00 e8 13 d6 db fc 49 8d bc 24 28 02 00 00 48 b8 00 00 00 00 00 fc ff df 48 89 f a 48 c1 ea 03 <80> 3c 02 00 0f 85 ed 02 00 00 4d 8b a4 24 28 02 00 00 e8 13 16 Fixes: 80d84ef3ff1dd ("l2tp: prevent l2tp_tunnel_delete racing with userspace close") Signed-off-by: James Chapman Signed-off-by: David S. Miller --- net/l2tp/l2tp_core.c | 117 +++++++++++++++------------------------------------ net/l2tp/l2tp_core.h | 23 +--------- net/l2tp/l2tp_ip.c | 10 ++--- net/l2tp/l2tp_ip6.c | 8 ++-- 4 files changed, 42 insertions(+), 116 deletions(-) (limited to 'net/l2tp') diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index 9cd2a99d0752..0fa53ead24aa 100644 --- a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c @@ -136,51 +136,6 @@ l2tp_session_id_hash_2(struct l2tp_net *pn, u32 session_id) } -/* Lookup the tunnel socket, possibly involving the fs code if the socket is - * owned by userspace. A struct sock returned from this function must be - * released using l2tp_tunnel_sock_put once you're done with it. - */ -static struct sock *l2tp_tunnel_sock_lookup(struct l2tp_tunnel *tunnel) -{ - int err = 0; - struct socket *sock = NULL; - struct sock *sk = NULL; - - if (!tunnel) - goto out; - - if (tunnel->fd >= 0) { - /* Socket is owned by userspace, who might be in the process - * of closing it. Look the socket up using the fd to ensure - * consistency. - */ - sock = sockfd_lookup(tunnel->fd, &err); - if (sock) - sk = sock->sk; - } else { - /* Socket is owned by kernelspace */ - sk = tunnel->sock; - sock_hold(sk); - } - -out: - return sk; -} - -/* Drop a reference to a tunnel socket obtained via. l2tp_tunnel_sock_put */ -static void l2tp_tunnel_sock_put(struct sock *sk) -{ - struct l2tp_tunnel *tunnel = l2tp_sock_to_tunnel(sk); - if (tunnel) { - if (tunnel->fd >= 0) { - /* Socket is owned by userspace */ - sockfd_put(sk->sk_socket); - } - sock_put(sk); - } - sock_put(sk); -} - /* Session hash list. * The session_id SHOULD be random according to RFC2661, but several * L2TP implementations (Cisco and Microsoft) use incrementing @@ -193,6 +148,13 @@ l2tp_session_id_hash(struct l2tp_tunnel *tunnel, u32 session_id) return &tunnel->session_hlist[hash_32(session_id, L2TP_HASH_BITS)]; } +void l2tp_tunnel_free(struct l2tp_tunnel *tunnel) +{ + sock_put(tunnel->sock); + /* the tunnel is freed in the socket destructor */ +} +EXPORT_SYMBOL(l2tp_tunnel_free); + /* Lookup a tunnel. A new reference is held on the returned tunnel. */ struct l2tp_tunnel *l2tp_tunnel_get(const struct net *net, u32 tunnel_id) { @@ -345,13 +307,11 @@ int l2tp_session_register(struct l2tp_session *session, } l2tp_tunnel_inc_refcount(tunnel); - sock_hold(tunnel->sock); hlist_add_head_rcu(&session->global_hlist, g_head); spin_unlock_bh(&pn->l2tp_session_hlist_lock); } else { l2tp_tunnel_inc_refcount(tunnel); - sock_hold(tunnel->sock); } hlist_add_head(&session->hlist, head); @@ -969,7 +929,7 @@ int l2tp_udp_encap_recv(struct sock *sk, struct sk_buff *skb) { struct l2tp_tunnel *tunnel; - tunnel = l2tp_sock_to_tunnel(sk); + tunnel = l2tp_tunnel(sk); if (tunnel == NULL) goto pass_up; @@ -977,13 +937,10 @@ int l2tp_udp_encap_recv(struct sock *sk, struct sk_buff *skb) tunnel->name, skb->len); if (l2tp_udp_recv_core(tunnel, skb, tunnel->recv_payload_hook)) - goto pass_up_put; + goto pass_up; - sock_put(sk); return 0; -pass_up_put: - sock_put(sk); pass_up: return 1; } @@ -1214,7 +1171,6 @@ static void l2tp_tunnel_destruct(struct sock *sk) l2tp_info(tunnel, L2TP_MSG_CONTROL, "%s: closing...\n", tunnel->name); - /* Disable udp encapsulation */ switch (tunnel->encap) { case L2TP_ENCAPTYPE_UDP: @@ -1237,12 +1193,11 @@ static void l2tp_tunnel_destruct(struct sock *sk) list_del_rcu(&tunnel->list); spin_unlock_bh(&pn->l2tp_tunnel_list_lock); - tunnel->sock = NULL; - l2tp_tunnel_dec_refcount(tunnel); - /* Call the original destructor */ if (sk->sk_destruct) (*sk->sk_destruct)(sk); + + kfree_rcu(tunnel, rcu); end: return; } @@ -1303,30 +1258,22 @@ EXPORT_SYMBOL_GPL(l2tp_tunnel_closeall); /* Tunnel socket destroy hook for UDP encapsulation */ static void l2tp_udp_encap_destroy(struct sock *sk) { - struct l2tp_tunnel *tunnel = l2tp_sock_to_tunnel(sk); - if (tunnel) { - l2tp_tunnel_closeall(tunnel); - sock_put(sk); - } + struct l2tp_tunnel *tunnel = l2tp_tunnel(sk); + + if (tunnel) + l2tp_tunnel_delete(tunnel); } /* Workqueue tunnel deletion function */ static void l2tp_tunnel_del_work(struct work_struct *work) { - struct l2tp_tunnel *tunnel = NULL; - struct socket *sock = NULL; - struct sock *sk = NULL; - - tunnel = container_of(work, struct l2tp_tunnel, del_work); + struct l2tp_tunnel *tunnel = container_of(work, struct l2tp_tunnel, + del_work); + struct sock *sk = tunnel->sock; + struct socket *sock = sk->sk_socket; l2tp_tunnel_closeall(tunnel); - sk = l2tp_tunnel_sock_lookup(tunnel); - if (!sk) - goto out; - - sock = sk->sk_socket; - /* If the tunnel socket was created within the kernel, use * the sk API to release it here. */ @@ -1337,8 +1284,10 @@ static void l2tp_tunnel_del_work(struct work_struct *work) } } - l2tp_tunnel_sock_put(sk); -out: + /* drop initial ref */ + l2tp_tunnel_dec_refcount(tunnel); + + /* drop workqueue ref */ l2tp_tunnel_dec_refcount(tunnel); } @@ -1591,13 +1540,22 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32 sk->sk_user_data = tunnel; } + /* Bump the reference count. The tunnel context is deleted + * only when this drops to zero. A reference is also held on + * the tunnel socket to ensure that it is not released while + * the tunnel is extant. Must be done before sk_destruct is + * set. + */ + refcount_set(&tunnel->ref_count, 1); + sock_hold(sk); + tunnel->sock = sk; + tunnel->fd = fd; + /* Hook on the tunnel socket destructor so that we can cleanup * if the tunnel socket goes away. */ tunnel->old_sk_destruct = sk->sk_destruct; sk->sk_destruct = &l2tp_tunnel_destruct; - tunnel->sock = sk; - tunnel->fd = fd; lockdep_set_class_and_name(&sk->sk_lock.slock, &l2tp_socket_class, "l2tp_sock"); sk->sk_allocation = GFP_ATOMIC; @@ -1607,11 +1565,6 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32 /* Add tunnel to our list */ INIT_LIST_HEAD(&tunnel->list); - - /* Bump the reference count. The tunnel context is deleted - * only when this drops to zero. Must be done before list insertion - */ - refcount_set(&tunnel->ref_count, 1); spin_lock_bh(&pn->l2tp_tunnel_list_lock); list_add_rcu(&tunnel->list, &pn->l2tp_tunnel_list); spin_unlock_bh(&pn->l2tp_tunnel_list_lock); @@ -1652,8 +1605,6 @@ void l2tp_session_free(struct l2tp_session *session) if (tunnel) { BUG_ON(tunnel->magic != L2TP_TUNNEL_MAGIC); - sock_put(tunnel->sock); - session->tunnel = NULL; l2tp_tunnel_dec_refcount(tunnel); } diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h index 9bbee90e9963..a1aa9550f04e 100644 --- a/net/l2tp/l2tp_core.h +++ b/net/l2tp/l2tp_core.h @@ -214,27 +214,8 @@ static inline void *l2tp_session_priv(struct l2tp_session *session) return &session->priv[0]; } -static inline struct l2tp_tunnel *l2tp_sock_to_tunnel(struct sock *sk) -{ - struct l2tp_tunnel *tunnel; - - if (sk == NULL) - return NULL; - - sock_hold(sk); - tunnel = (struct l2tp_tunnel *)(sk->sk_user_data); - if (tunnel == NULL) { - sock_put(sk); - goto out; - } - - BUG_ON(tunnel->magic != L2TP_TUNNEL_MAGIC); - -out: - return tunnel; -} - struct l2tp_tunnel *l2tp_tunnel_get(const struct net *net, u32 tunnel_id); +void l2tp_tunnel_free(struct l2tp_tunnel *tunnel); struct l2tp_session *l2tp_session_get(const struct net *net, struct l2tp_tunnel *tunnel, @@ -283,7 +264,7 @@ static inline void l2tp_tunnel_inc_refcount(struct l2tp_tunnel *tunnel) static inline void l2tp_tunnel_dec_refcount(struct l2tp_tunnel *tunnel) { if (refcount_dec_and_test(&tunnel->ref_count)) - kfree_rcu(tunnel, rcu); + l2tp_tunnel_free(tunnel); } /* Session reference counts. Incremented when code obtains a reference diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c index ff61124fdf59..3428fba6f2b7 100644 --- a/net/l2tp/l2tp_ip.c +++ b/net/l2tp/l2tp_ip.c @@ -234,17 +234,13 @@ 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 = sk->sk_user_data; while ((skb = __skb_dequeue_tail(&sk->sk_write_queue)) != NULL) kfree_skb(skb); - if (tunnel) { - l2tp_tunnel_closeall(tunnel); - sock_put(sk); - } - - sk_refcnt_debug_dec(sk); + if (tunnel) + l2tp_tunnel_delete(tunnel); } static int l2tp_ip_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len) diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c index 192344688c06..6f009eaa5fbe 100644 --- a/net/l2tp/l2tp_ip6.c +++ b/net/l2tp/l2tp_ip6.c @@ -248,16 +248,14 @@ 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 = sk->sk_user_data; lock_sock(sk); ip6_flush_pending_frames(sk); release_sock(sk); - if (tunnel) { - l2tp_tunnel_closeall(tunnel); - sock_put(sk); - } + if (tunnel) + l2tp_tunnel_delete(tunnel); inet6_destroy_sock(sk); } -- cgit v1.2.3-59-g8ed1b From d02ba2a6110c530a32926af8ad441111774d2893 Mon Sep 17 00:00:00 2001 From: James Chapman Date: Fri, 23 Feb 2018 17:45:46 +0000 Subject: l2tp: fix race in pppol2tp_release with session object destroy pppol2tp_release uses call_rcu to put the final ref on its socket. But the session object doesn't hold a ref on the session socket so may be freed while the pppol2tp_put_sk RCU callback is scheduled. Fix this by having the session hold a ref on its socket until the session is destroyed. It is this ref that is dropped via call_rcu. Sessions are also deleted via l2tp_tunnel_closeall. This must now also put the final ref via call_rcu. So move the call_rcu call site into pppol2tp_session_close so that this happens in both destroy paths. A common destroy path should really be implemented, perhaps with l2tp_tunnel_closeall calling l2tp_session_delete like pppol2tp_release does, but this will be looked at later. ODEBUG: activate active (active state 1) object type: rcu_head hint: (null) WARNING: CPU: 3 PID: 13407 at lib/debugobjects.c:291 debug_print_object+0x166/0x220 Modules linked in: CPU: 3 PID: 13407 Comm: syzbot_19c09769 Not tainted 4.16.0-rc2+ #38 Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006 RIP: 0010:debug_print_object+0x166/0x220 RSP: 0018:ffff880013647a00 EFLAGS: 00010082 RAX: dffffc0000000008 RBX: 0000000000000003 RCX: ffffffff814d3333 RDX: 0000000000000000 RSI: 0000000000000001 RDI: ffff88001a59f6d0 RBP: ffff880013647a40 R08: 0000000000000000 R09: 0000000000000001 R10: ffff8800136479a8 R11: 0000000000000000 R12: 0000000000000001 R13: ffffffff86161420 R14: ffffffff85648b60 R15: 0000000000000000 FS: 0000000000000000(0000) GS:ffff88001a580000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000020e77000 CR3: 0000000006022000 CR4: 00000000000006e0 Call Trace: debug_object_activate+0x38b/0x530 ? debug_object_assert_init+0x3b0/0x3b0 ? __mutex_unlock_slowpath+0x85/0x8b0 ? pppol2tp_session_destruct+0x110/0x110 __call_rcu.constprop.66+0x39/0x890 ? __call_rcu.constprop.66+0x39/0x890 call_rcu_sched+0x17/0x20 pppol2tp_release+0x2c7/0x440 ? fcntl_setlk+0xca0/0xca0 ? sock_alloc_file+0x340/0x340 sock_release+0x92/0x1e0 sock_close+0x1b/0x20 __fput+0x296/0x6e0 ____fput+0x1a/0x20 task_work_run+0x127/0x1a0 do_exit+0x7f9/0x2ce0 ? SYSC_connect+0x212/0x310 ? mm_update_next_owner+0x690/0x690 ? up_read+0x1f/0x40 ? __do_page_fault+0x3c8/0xca0 do_group_exit+0x10d/0x330 ? do_group_exit+0x330/0x330 SyS_exit_group+0x22/0x30 do_syscall_64+0x1e0/0x730 ? trace_hardirqs_off_thunk+0x1a/0x1c entry_SYSCALL_64_after_hwframe+0x42/0xb7 RIP: 0033:0x7f362e471259 RSP: 002b:00007ffe389abe08 EFLAGS: 00000202 ORIG_RAX: 00000000000000e7 RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f362e471259 RDX: 00007f362e471259 RSI: 000000000000002e RDI: 0000000000000000 RBP: 00007ffe389abe30 R08: 0000000000000000 R09: 00007f362e944270 R10: 0000000000000000 R11: 0000000000000202 R12: 0000000000400b60 R13: 00007ffe389abf50 R14: 0000000000000000 R15: 0000000000000000 Code: 8d 3c dd a0 8f 64 85 48 89 fa 48 c1 ea 03 80 3c 02 00 75 7b 48 8b 14 dd a0 8f 64 85 4c 89 f6 48 c7 c7 20 85 64 85 e 8 2a 55 14 ff <0f> 0b 83 05 ad 2a 68 04 01 48 83 c4 18 5b 41 5c 41 5d 41 5e 41 Fixes: ee40fb2e1eb5b ("l2tp: protect sock pointer of struct pppol2tp_session with RCU") Signed-off-by: James Chapman Signed-off-by: David S. Miller --- net/l2tp/l2tp_ppp.c | 52 +++++++++++++++++++++++++++------------------------- 1 file changed, 27 insertions(+), 25 deletions(-) (limited to 'net/l2tp') diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c index 2d2955e8f710..3b02f24ea9ec 100644 --- a/net/l2tp/l2tp_ppp.c +++ b/net/l2tp/l2tp_ppp.c @@ -416,10 +416,28 @@ abort: * Session (and tunnel control) socket create/destroy. *****************************************************************************/ +static void pppol2tp_put_sk(struct rcu_head *head) +{ + struct pppol2tp_session *ps; + + ps = container_of(head, typeof(*ps), rcu); + sock_put(ps->__sk); +} + /* Called by l2tp_core when a session socket is being closed. */ static void pppol2tp_session_close(struct l2tp_session *session) { + struct pppol2tp_session *ps; + + ps = l2tp_session_priv(session); + mutex_lock(&ps->sk_lock); + ps->__sk = rcu_dereference_protected(ps->sk, + lockdep_is_held(&ps->sk_lock)); + RCU_INIT_POINTER(ps->sk, NULL); + if (ps->__sk) + call_rcu(&ps->rcu, pppol2tp_put_sk); + mutex_unlock(&ps->sk_lock); } /* Really kill the session socket. (Called from sock_put() if @@ -439,14 +457,6 @@ static void pppol2tp_session_destruct(struct sock *sk) } } -static void pppol2tp_put_sk(struct rcu_head *head) -{ - struct pppol2tp_session *ps; - - ps = container_of(head, typeof(*ps), rcu); - sock_put(ps->__sk); -} - /* Called when the PPPoX socket (session) is closed. */ static int pppol2tp_release(struct socket *sock) @@ -470,26 +480,17 @@ static int pppol2tp_release(struct socket *sock) sock_orphan(sk); sock->sk = NULL; + /* If the socket is associated with a session, + * l2tp_session_delete will call pppol2tp_session_close which + * will drop the session's ref on the socket. + */ session = pppol2tp_sock_to_session(sk); - - if (session != NULL) { - struct pppol2tp_session *ps; - + if (session) { l2tp_session_delete(session); - - ps = l2tp_session_priv(session); - mutex_lock(&ps->sk_lock); - ps->__sk = rcu_dereference_protected(ps->sk, - lockdep_is_held(&ps->sk_lock)); - RCU_INIT_POINTER(ps->sk, NULL); - mutex_unlock(&ps->sk_lock); - call_rcu(&ps->rcu, pppol2tp_put_sk); - - /* Rely on the sock_put() call at the end of the function for - * dropping the reference held by pppol2tp_sock_to_session(). - * The last reference will be dropped by pppol2tp_put_sk(). - */ + /* drop the ref obtained by pppol2tp_sock_to_session */ + sock_put(sk); } + release_sock(sk); /* This will delete the session context via @@ -786,6 +787,7 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr, out_no_ppp: /* This is how we get the session context from the socket. */ + sock_hold(sk); sk->sk_user_data = session; rcu_assign_pointer(ps->sk, sk); mutex_unlock(&ps->sk_lock); -- cgit v1.2.3-59-g8ed1b From 28f5bfb819195ad9c2eb9486babe7b0e4efe925f Mon Sep 17 00:00:00 2001 From: James Chapman Date: Fri, 23 Feb 2018 17:45:47 +0000 Subject: l2tp: fix tunnel lookup use-after-free race l2tp_tunnel_get walks the tunnel list to find a matching tunnel instance and if a match is found, its refcount is increased before returning the tunnel pointer. But when tunnel objects are destroyed, they are on the tunnel list after their refcount hits zero. Fix this by moving the code that removes the tunnel from the tunnel list from the tunnel socket destructor into in the l2tp_tunnel_delete path, before the tunnel refcount is decremented. refcount_t: increment on 0; use-after-free. WARNING: CPU: 3 PID: 13507 at lib/refcount.c:153 refcount_inc+0x47/0x50 Modules linked in: CPU: 3 PID: 13507 Comm: syzbot_6e6a5ec8 Not tainted 4.16.0-rc2+ #36 Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006 RIP: 0010:refcount_inc+0x47/0x50 RSP: 0018:ffff8800136ffb20 EFLAGS: 00010286 RAX: dffffc0000000008 RBX: ffff880017068e68 RCX: ffffffff814d3333 RDX: 0000000000000000 RSI: ffff88001a59f6d8 RDI: ffff88001a59f6d8 RBP: ffff8800136ffb28 R08: 0000000000000000 R09: 0000000000000000 R10: ffff8800136ffab0 R11: 0000000000000000 R12: ffff880017068e50 R13: 0000000000000000 R14: ffff8800174da800 R15: 0000000000000004 FS: 00007f403ab1e700(0000) GS:ffff88001a580000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00000000205fafd2 CR3: 0000000016770000 CR4: 00000000000006e0 Call Trace: l2tp_tunnel_get+0x2dd/0x4e0 pppol2tp_connect+0x428/0x13c0 ? pppol2tp_session_create+0x170/0x170 ? __might_fault+0x115/0x1d0 ? lock_downgrade+0x860/0x860 ? __might_fault+0xe5/0x1d0 ? security_socket_connect+0x8e/0xc0 SYSC_connect+0x1b6/0x310 ? SYSC_bind+0x280/0x280 ? __do_page_fault+0x5d1/0xca0 ? up_read+0x1f/0x40 ? __do_page_fault+0x3c8/0xca0 SyS_connect+0x29/0x30 ? SyS_accept+0x40/0x40 do_syscall_64+0x1e0/0x730 ? trace_hardirqs_off_thunk+0x1a/0x1c entry_SYSCALL_64_after_hwframe+0x42/0xb7 RIP: 0033:0x7f403a42f259 RSP: 002b:00007f403ab1dee8 EFLAGS: 00000296 ORIG_RAX: 000000000000002a RAX: ffffffffffffffda RBX: 00000000205fafe4 RCX: 00007f403a42f259 RDX: 000000000000002e RSI: 00000000205fafd2 RDI: 0000000000000004 RBP: 00007f403ab1df20 R08: 00007f403ab1e700 R09: 0000000000000000 R10: 00007f403ab1e700 R11: 0000000000000296 R12: 0000000000000000 R13: 00007ffc81906cbf R14: 0000000000000000 R15: 00007f403ab2b040 Code: 3b ff 5b 5d c3 e8 ca 5f 3b ff 80 3d 49 8e 66 04 00 75 ea e8 bc 5f 3b ff 48 c7 c7 60 69 64 85 c6 05 34 8e 66 04 01 e8 59 49 15 ff <0f> 0b eb ce 0f 1f 44 00 00 55 48 89 e5 41 56 41 55 41 54 53 49 Fixes: f8ccac0e44934 ("l2tp: put tunnel socket release on a workqueue") Reported-and-tested-by: syzbot+19c09769f14b48810113@syzkaller.appspotmail.com Reported-and-tested-by: syzbot+347bd5acde002e353a36@syzkaller.appspotmail.com Reported-and-tested-by: syzbot+6e6a5ec8de31a94cd015@syzkaller.appspotmail.com Reported-and-tested-by: syzbot+9df43faf09bd400f2993@syzkaller.appspotmail.com Signed-off-by: James Chapman Signed-off-by: David S. Miller --- net/l2tp/l2tp_core.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'net/l2tp') diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index 0fa53ead24aa..83421c6f0bef 100644 --- a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c @@ -1164,7 +1164,6 @@ EXPORT_SYMBOL_GPL(l2tp_xmit_skb); static void l2tp_tunnel_destruct(struct sock *sk) { struct l2tp_tunnel *tunnel = l2tp_tunnel(sk); - struct l2tp_net *pn; if (tunnel == NULL) goto end; @@ -1187,12 +1186,6 @@ static void l2tp_tunnel_destruct(struct sock *sk) sk->sk_destruct = tunnel->old_sk_destruct; sk->sk_user_data = NULL; - /* Remove the tunnel struct from the tunnel list */ - pn = l2tp_pernet(tunnel->l2tp_net); - spin_lock_bh(&pn->l2tp_tunnel_list_lock); - list_del_rcu(&tunnel->list); - spin_unlock_bh(&pn->l2tp_tunnel_list_lock); - /* Call the original destructor */ if (sk->sk_destruct) (*sk->sk_destruct)(sk); @@ -1271,6 +1264,7 @@ static void l2tp_tunnel_del_work(struct work_struct *work) del_work); struct sock *sk = tunnel->sock; struct socket *sock = sk->sk_socket; + struct l2tp_net *pn; l2tp_tunnel_closeall(tunnel); @@ -1284,6 +1278,12 @@ static void l2tp_tunnel_del_work(struct work_struct *work) } } + /* Remove the tunnel struct from the tunnel list */ + pn = l2tp_pernet(tunnel->l2tp_net); + spin_lock_bh(&pn->l2tp_tunnel_list_lock); + list_del_rcu(&tunnel->list); + spin_unlock_bh(&pn->l2tp_tunnel_list_lock); + /* drop initial ref */ l2tp_tunnel_dec_refcount(tunnel); -- cgit v1.2.3-59-g8ed1b