lkml.org 
[lkml]   [2018]   [Jun]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    Subject[PATCH 3.16 292/410] l2tp: fix races with tunnel socket close
    3.16.57-rc1 review patch.  If anyone has any objections, please let me know.

    ------------------

    From: James Chapman <jchapman@katalix.com>

    commit d00fa9adc528c1b0e64d532556764852df8bd7b9 upstream.

    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 <jchapman@katalix.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>
    [bwh; Backported to 3.16: adjust context]
    Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
    ---
    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(-)

    --- a/net/l2tp/l2tp_core.c
    +++ b/net/l2tp/l2tp_core.c
    @@ -135,51 +135,6 @@ l2tp_session_id_hash_2(struct l2tp_net *

    }

    -/* 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);
    -}
    -
    /* Lookup a session by id in the global session list
    */
    static struct l2tp_session *l2tp_session_find_2(struct net *net, u32 session_id)
    @@ -241,6 +196,13 @@ struct l2tp_session *l2tp_session_find(s
    }
    EXPORT_SYMBOL_GPL(l2tp_session_find);

    +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)
    {
    @@ -405,13 +367,11 @@ int l2tp_session_register(struct l2tp_se
    }

    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);
    @@ -1051,7 +1011,7 @@ int l2tp_udp_encap_recv(struct sock *sk,
    {
    struct l2tp_tunnel *tunnel;

    - tunnel = l2tp_sock_to_tunnel(sk);
    + tunnel = l2tp_tunnel(sk);
    if (tunnel == NULL)
    goto pass_up;

    @@ -1059,13 +1019,10 @@ int l2tp_udp_encap_recv(struct sock *sk,
    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;
    }
    @@ -1299,7 +1256,6 @@ static void l2tp_tunnel_destruct(struct

    l2tp_info(tunnel, L2TP_MSG_CONTROL, "%s: closing...\n", tunnel->name);

    -
    /* Disable udp encapsulation */
    switch (tunnel->encap) {
    case L2TP_ENCAPTYPE_UDP:
    @@ -1322,12 +1278,11 @@ static void l2tp_tunnel_destruct(struct
    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;
    }
    @@ -1391,30 +1346,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.
    */
    @@ -1424,8 +1371,10 @@ static void l2tp_tunnel_del_work(struct
    sk_release_kernel(sk);
    }

    - l2tp_tunnel_sock_put(sk);
    -out:
    + /* drop initial ref */
    + l2tp_tunnel_dec_refcount(tunnel);
    +
    + /* drop workqueue ref */
    l2tp_tunnel_dec_refcount(tunnel);
    }

    @@ -1717,13 +1666,22 @@ int l2tp_tunnel_create(struct net *net,

    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.
    + */
    + atomic_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;
    @@ -1733,11 +1691,6 @@ int l2tp_tunnel_create(struct net *net,

    /* 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
    - */
    - l2tp_tunnel_inc_refcount(tunnel);
    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);
    @@ -1778,8 +1731,6 @@ void l2tp_session_free(struct l2tp_sessi

    if (tunnel) {
    BUG_ON(tunnel->magic != L2TP_TUNNEL_MAGIC);
    - sock_put(tunnel->sock);
    - session->tunnel = NULL;
    l2tp_tunnel_dec_refcount(tunnel);
    }

    --- a/net/l2tp/l2tp_core.h
    +++ b/net/l2tp/l2tp_core.h
    @@ -229,27 +229,8 @@ static inline void *l2tp_session_priv(st
    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,
    @@ -303,7 +284,7 @@ static inline void l2tp_tunnel_inc_refco
    static inline void l2tp_tunnel_dec_refcount(struct l2tp_tunnel *tunnel)
    {
    if (atomic_dec_and_test(&tunnel->ref_count))
    - kfree_rcu(tunnel, rcu);
    + l2tp_tunnel_free(tunnel);
    }

    /* Session reference counts. Incremented when code obtains a reference
    --- a/net/l2tp/l2tp_ip.c
    +++ b/net/l2tp/l2tp_ip.c
    @@ -234,17 +234,13 @@ static void l2tp_ip_close(struct sock *s
    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)
    --- a/net/l2tp/l2tp_ip6.c
    +++ b/net/l2tp/l2tp_ip6.c
    @@ -246,16 +246,14 @@ static void l2tp_ip6_close(struct sock *

    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);
    }
    \
     
     \ /
      Last update: 2018-06-07 16:20    [W:4.495 / U:0.312 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site