Skip to content

Commit

Permalink
l2tp: close all race conditions in l2tp_tunnel_register()
Browse files Browse the repository at this point in the history
The code in l2tp_tunnel_register() is racy in several ways:

1. It modifies the tunnel socket _after_ publishing it.

2. It calls setup_udp_tunnel_sock() on an existing socket without
   locking.

3. It changes sock lock class on fly, which triggers many syzbot
   reports.

This patch amends all of them by moving socket initialization code
before publishing and under sock lock. As suggested by Jakub, the
l2tp lockdep class is not necessary as we can just switch to
bh_lock_sock_nested().

Fixes: 37159ef ("l2tp: fix a lockdep splat")
Fixes: 6b9f342 ("l2tp: fix races in tunnel creation")
Reported-by: [email protected]
Reported-by: [email protected]
Reported-by: Tetsuo Handa <[email protected]>
Cc: Guillaume Nault <[email protected]>
Cc: Jakub Sitnicki <[email protected]>
Cc: Eric Dumazet <[email protected]>
Cc: Tom Parkin <[email protected]>
Signed-off-by: Cong Wang <[email protected]>
Reviewed-by: Guillaume Nault <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
  • Loading branch information
Cong Wang authored and davem330 committed Jan 16, 2023
1 parent c4d48a5 commit 0b2c597
Showing 1 changed file with 14 additions and 14 deletions.
28 changes: 14 additions & 14 deletions net/l2tp/l2tp_core.c
Original file line number Diff line number Diff line change
Expand Up @@ -1041,7 +1041,7 @@ static int l2tp_xmit_core(struct l2tp_session *session, struct sk_buff *skb, uns
IPCB(skb)->flags &= ~(IPSKB_XFRM_TUNNEL_SIZE | IPSKB_XFRM_TRANSFORMED | IPSKB_REROUTED);
nf_reset_ct(skb);

bh_lock_sock(sk);
bh_lock_sock_nested(sk);
if (sock_owned_by_user(sk)) {
kfree_skb(skb);
ret = NET_XMIT_DROP;
Expand Down Expand Up @@ -1385,8 +1385,6 @@ static int l2tp_tunnel_sock_create(struct net *net,
return err;
}

static struct lock_class_key l2tp_socket_class;

int l2tp_tunnel_create(int fd, int version, u32 tunnel_id, u32 peer_tunnel_id,
struct l2tp_tunnel_cfg *cfg, struct l2tp_tunnel **tunnelp)
{
Expand Down Expand Up @@ -1482,21 +1480,16 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
}

sk = sock->sk;
lock_sock(sk);
write_lock_bh(&sk->sk_callback_lock);
ret = l2tp_validate_socket(sk, net, tunnel->encap);
if (ret < 0)
if (ret < 0) {
release_sock(sk);
goto err_inval_sock;
}
rcu_assign_sk_user_data(sk, tunnel);
write_unlock_bh(&sk->sk_callback_lock);

sock_hold(sk);
tunnel->sock = sk;
tunnel->l2tp_net = net;

spin_lock_bh(&pn->l2tp_tunnel_idr_lock);
idr_replace(&pn->l2tp_tunnel_idr, tunnel, tunnel->tunnel_id);
spin_unlock_bh(&pn->l2tp_tunnel_idr_lock);

if (tunnel->encap == L2TP_ENCAPTYPE_UDP) {
struct udp_tunnel_sock_cfg udp_cfg = {
.sk_user_data = tunnel,
Expand All @@ -1510,9 +1503,16 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,

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");
sk->sk_allocation = GFP_ATOMIC;
release_sock(sk);

sock_hold(sk);
tunnel->sock = sk;
tunnel->l2tp_net = net;

spin_lock_bh(&pn->l2tp_tunnel_idr_lock);
idr_replace(&pn->l2tp_tunnel_idr, tunnel, tunnel->tunnel_id);
spin_unlock_bh(&pn->l2tp_tunnel_idr_lock);

trace_register_tunnel(tunnel);

Expand Down

0 comments on commit 0b2c597

Please sign in to comment.