3.16.60-rc1 review patch. If anyone has any objections, please let me know.
------------------
From: Guillaume Nault g.nault@alphalink.fr
commit 6b9f34239b00e6956a267abed2bc559ede556ad6 upstream.
l2tp_tunnel_create() inserts the new tunnel into the namespace's tunnel list and sets the socket's ->sk_user_data field, before returning it to the caller. Therefore, there are two ways the tunnel can be accessed and freed, before the caller even had the opportunity to take a reference. In practice, syzbot could crash the module by closing the socket right after a new tunnel was returned to pppol2tp_create().
This patch moves tunnel registration out of l2tp_tunnel_create(), so that the caller can safely hold a reference before publishing the tunnel. This second step is done with the new l2tp_tunnel_register() function, which is now responsible for associating the tunnel to its socket and for inserting it into the namespace's list.
While moving the code to l2tp_tunnel_register(), a few modifications have been done. First, the socket validation tests are done in a helper function, for clarity. Also, modifying the socket is now done after having inserted the tunnel to the namespace's tunnels list. This will allow insertion to fail, without having to revert theses modifications in the error path (a followup patch will check for duplicate tunnels before insertion). Either the socket is a kernel socket which we control, or it is a user-space socket for which we have a reference on the file descriptor. In any case, the socket isn't going to be closed from under us.
Reported-by: syzbot+fbeeb5c3b538e8545644@syzkaller.appspotmail.com Fixes: fd558d186df2 ("l2tp: Split pppol2tp patch into separate l2tp and ppp parts") Signed-off-by: Guillaume Nault g.nault@alphalink.fr Signed-off-by: David S. Miller davem@davemloft.net [bwh: Backported to 3.16: - Socket setup is open-coded rather than using setup_udp_tunnel_sock() - l2tp_nl_cmd_tunnel_create() doesn't call l2tp_tunnel_notify() Signed-off-by: Ben Hutchings ben@decadent.org.uk --- net/l2tp/l2tp_core.c | 192 ++++++++++++++++++---------------------- net/l2tp/l2tp_core.h | 3 + net/l2tp/l2tp_netlink.c | 16 +++- net/l2tp/l2tp_ppp.c | 9 ++ 4 files changed, 110 insertions(+), 110 deletions(-)
--- a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c @@ -1560,74 +1560,11 @@ int l2tp_tunnel_create(struct net *net, { struct l2tp_tunnel *tunnel = NULL; int err; - struct socket *sock = NULL; - struct sock *sk = NULL; - struct l2tp_net *pn; enum l2tp_encap_type encap = L2TP_ENCAPTYPE_UDP;
- /* Get the tunnel socket from the fd, which was opened by - * the userspace L2TP daemon. If not specified, create a - * kernel socket. - */ - if (fd < 0) { - err = l2tp_tunnel_sock_create(net, tunnel_id, peer_tunnel_id, - cfg, &sock); - if (err < 0) - goto err; - } else { - sock = sockfd_lookup(fd, &err); - if (!sock) { - pr_err("tunl %u: sockfd_lookup(fd=%d) returned %d\n", - tunnel_id, fd, err); - err = -EBADF; - goto err; - } - - /* Reject namespace mismatches */ - if (!net_eq(sock_net(sock->sk), net)) { - pr_err("tunl %u: netns mismatch\n", tunnel_id); - err = -EINVAL; - goto err; - } - } - - sk = sock->sk; - if (cfg != NULL) encap = cfg->encap;
- /* Quick sanity checks */ - err = -EPROTONOSUPPORT; - if (sk->sk_type != SOCK_DGRAM) { - pr_debug("tunl %hu: fd %d wrong socket type\n", - tunnel_id, fd); - goto err; - } - switch (encap) { - case L2TP_ENCAPTYPE_UDP: - if (sk->sk_protocol != IPPROTO_UDP) { - pr_err("tunl %hu: fd %d wrong protocol, got %d, expected %d\n", - tunnel_id, fd, sk->sk_protocol, IPPROTO_UDP); - goto err; - } - break; - case L2TP_ENCAPTYPE_IP: - if (sk->sk_protocol != IPPROTO_L2TP) { - pr_err("tunl %hu: fd %d wrong protocol, got %d, expected %d\n", - tunnel_id, fd, sk->sk_protocol, IPPROTO_L2TP); - goto err; - } - break; - } - - /* Check if this socket has already been prepped */ - tunnel = l2tp_tunnel(sk); - if (tunnel != NULL) { - /* This socket has already been prepped */ - err = -EBUSY; - goto err; - } - tunnel = kzalloc(sizeof(struct l2tp_tunnel), GFP_KERNEL); if (tunnel == NULL) { err = -ENOMEM; @@ -1644,17 +1581,83 @@ int l2tp_tunnel_create(struct net *net, rwlock_init(&tunnel->hlist_lock); tunnel->acpt_newsess = true;
- /* The net we belong to */ - tunnel->l2tp_net = net; - pn = l2tp_pernet(net); - if (cfg != NULL) tunnel->debug = cfg->debug;
- /* Mark socket as an encapsulation socket. See net/ipv4/udp.c */ tunnel->encap = encap; - if (encap == L2TP_ENCAPTYPE_UDP) { - /* Mark socket as an encapsulation socket. See net/ipv4/udp.c */ + + atomic_set(&tunnel->ref_count, 1); + tunnel->fd = fd; + + /* Init delete workqueue struct */ + INIT_WORK(&tunnel->del_work, l2tp_tunnel_del_work); + + INIT_LIST_HEAD(&tunnel->list); + + err = 0; +err: + if (tunnelp) + *tunnelp = tunnel; + + return err; +} +EXPORT_SYMBOL_GPL(l2tp_tunnel_create); + +static int l2tp_validate_socket(const struct sock *sk, const struct net *net, + enum l2tp_encap_type encap) +{ + if (!net_eq(sock_net(sk), net)) + return -EINVAL; + + if (sk->sk_type != SOCK_DGRAM) + return -EPROTONOSUPPORT; + + if ((encap == L2TP_ENCAPTYPE_UDP && sk->sk_protocol != IPPROTO_UDP) || + (encap == L2TP_ENCAPTYPE_IP && sk->sk_protocol != IPPROTO_L2TP)) + return -EPROTONOSUPPORT; + + if (sk->sk_user_data) + return -EBUSY; + + return 0; +} + +int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, + struct l2tp_tunnel_cfg *cfg) +{ + struct l2tp_net *pn; + struct socket *sock; + struct sock *sk; + int ret; + + if (tunnel->fd < 0) { + ret = l2tp_tunnel_sock_create(net, tunnel->tunnel_id, + tunnel->peer_tunnel_id, cfg, + &sock); + if (ret < 0) + goto err; + } else { + sock = sockfd_lookup(tunnel->fd, &ret); + if (!sock) + goto err; + + ret = l2tp_validate_socket(sock->sk, net, tunnel->encap); + if (ret < 0) + goto err_sock; + } + + sk = sock->sk; + + sock_hold(sk); + tunnel->sock = sk; + tunnel->l2tp_net = net; + + pn = l2tp_pernet(net); + 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); + + if (tunnel->encap == L2TP_ENCAPTYPE_UDP) { udp_sk(sk)->encap_type = UDP_ENCAP_L2TPINUDP; udp_sk(sk)->encap_rcv = l2tp_udp_encap_recv; udp_sk(sk)->encap_destroy = l2tp_udp_encap_destroy; @@ -1668,49 +1671,23 @@ 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; - lockdep_set_class_and_name(&sk->sk_lock.slock, &l2tp_socket_class, "l2tp_sock"); - + lockdep_set_class_and_name(&sk->sk_lock.slock, &l2tp_socket_class, + "l2tp_sock"); sk->sk_allocation = GFP_ATOMIC;
- /* Init delete workqueue struct */ - INIT_WORK(&tunnel->del_work, l2tp_tunnel_del_work); + if (tunnel->fd >= 0) + sockfd_put(sock);
- /* Add tunnel to our list */ - INIT_LIST_HEAD(&tunnel->list); - 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); + return 0;
- err = 0; +err_sock: + sockfd_put(sock); err: - if (tunnelp) - *tunnelp = tunnel; - - /* If tunnel's socket was created by the kernel, it doesn't - * have a file. - */ - if (sock && sock->file) - sockfd_put(sock); - - return err; + return ret; } -EXPORT_SYMBOL_GPL(l2tp_tunnel_create); +EXPORT_SYMBOL_GPL(l2tp_tunnel_register);
/* This function is used by the netlink TUNNEL_DELETE command. */ --- a/net/l2tp/l2tp_core.h +++ b/net/l2tp/l2tp_core.h @@ -246,6 +246,9 @@ struct l2tp_tunnel *l2tp_tunnel_find_nth int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32 peer_tunnel_id, struct l2tp_tunnel_cfg *cfg, struct l2tp_tunnel **tunnelp); +int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, + struct l2tp_tunnel_cfg *cfg); + void l2tp_tunnel_closeall(struct l2tp_tunnel *tunnel); void l2tp_tunnel_delete(struct l2tp_tunnel *tunnel); struct l2tp_session *l2tp_session_create(int priv_size, --- a/net/l2tp/l2tp_netlink.c +++ b/net/l2tp/l2tp_netlink.c @@ -192,6 +192,17 @@ static int l2tp_nl_cmd_tunnel_create(str break; }
+ if (ret < 0) + goto out; + + l2tp_tunnel_inc_refcount(tunnel); + ret = l2tp_tunnel_register(tunnel, net, &cfg); + if (ret < 0) { + kfree(tunnel); + goto out; + } + l2tp_tunnel_dec_refcount(tunnel); + out: return ret; } --- a/net/l2tp/l2tp_ppp.c +++ b/net/l2tp/l2tp_ppp.c @@ -720,6 +720,15 @@ static int pppol2tp_connect(struct socke error = l2tp_tunnel_create(sock_net(sk), fd, ver, tunnel_id, peer_tunnel_id, &tcfg, &tunnel); if (error < 0) goto end; + + l2tp_tunnel_inc_refcount(tunnel); + error = l2tp_tunnel_register(tunnel, sock_net(sk), + &tcfg); + if (error < 0) { + kfree(tunnel); + goto end; + } + drop_tunnel = true; } } else { /* Error if we can't find the tunnel */