Skip to content

Commit 0b2c597

Browse files
Cong Wangdavem330
Cong Wang
authored andcommittedJan 16, 2023
l2tp: close all race conditions in l2tp_tunnel_register()
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: syzbot+52866e24647f9a23403f@syzkaller.appspotmail.com Reported-by: syzbot+94cc2a66fc228b23f360@syzkaller.appspotmail.com Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Cc: Guillaume Nault <gnault@redhat.com> Cc: Jakub Sitnicki <jakub@cloudflare.com> Cc: Eric Dumazet <edumazet@google.com> Cc: Tom Parkin <tparkin@katalix.com> Signed-off-by: Cong Wang <cong.wang@bytedance.com> Reviewed-by: Guillaume Nault <gnault@redhat.com> Signed-off-by: David S. Miller <davem@davemloft.net>
1 parent c4d48a5 commit 0b2c597

File tree

1 file changed

+14
-14
lines changed

1 file changed

+14
-14
lines changed
 

‎net/l2tp/l2tp_core.c

+14-14
Original file line numberDiff line numberDiff line change
@@ -1041,7 +1041,7 @@ static int l2tp_xmit_core(struct l2tp_session *session, struct sk_buff *skb, uns
10411041
IPCB(skb)->flags &= ~(IPSKB_XFRM_TUNNEL_SIZE | IPSKB_XFRM_TRANSFORMED | IPSKB_REROUTED);
10421042
nf_reset_ct(skb);
10431043

1044-
bh_lock_sock(sk);
1044+
bh_lock_sock_nested(sk);
10451045
if (sock_owned_by_user(sk)) {
10461046
kfree_skb(skb);
10471047
ret = NET_XMIT_DROP;
@@ -1385,8 +1385,6 @@ static int l2tp_tunnel_sock_create(struct net *net,
13851385
return err;
13861386
}
13871387

1388-
static struct lock_class_key l2tp_socket_class;
1389-
13901388
int l2tp_tunnel_create(int fd, int version, u32 tunnel_id, u32 peer_tunnel_id,
13911389
struct l2tp_tunnel_cfg *cfg, struct l2tp_tunnel **tunnelp)
13921390
{
@@ -1482,21 +1480,16 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
14821480
}
14831481

14841482
sk = sock->sk;
1483+
lock_sock(sk);
14851484
write_lock_bh(&sk->sk_callback_lock);
14861485
ret = l2tp_validate_socket(sk, net, tunnel->encap);
1487-
if (ret < 0)
1486+
if (ret < 0) {
1487+
release_sock(sk);
14881488
goto err_inval_sock;
1489+
}
14891490
rcu_assign_sk_user_data(sk, tunnel);
14901491
write_unlock_bh(&sk->sk_callback_lock);
14911492

1492-
sock_hold(sk);
1493-
tunnel->sock = sk;
1494-
tunnel->l2tp_net = net;
1495-
1496-
spin_lock_bh(&pn->l2tp_tunnel_idr_lock);
1497-
idr_replace(&pn->l2tp_tunnel_idr, tunnel, tunnel->tunnel_id);
1498-
spin_unlock_bh(&pn->l2tp_tunnel_idr_lock);
1499-
15001493
if (tunnel->encap == L2TP_ENCAPTYPE_UDP) {
15011494
struct udp_tunnel_sock_cfg udp_cfg = {
15021495
.sk_user_data = tunnel,
@@ -1510,9 +1503,16 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
15101503

15111504
tunnel->old_sk_destruct = sk->sk_destruct;
15121505
sk->sk_destruct = &l2tp_tunnel_destruct;
1513-
lockdep_set_class_and_name(&sk->sk_lock.slock, &l2tp_socket_class,
1514-
"l2tp_sock");
15151506
sk->sk_allocation = GFP_ATOMIC;
1507+
release_sock(sk);
1508+
1509+
sock_hold(sk);
1510+
tunnel->sock = sk;
1511+
tunnel->l2tp_net = net;
1512+
1513+
spin_lock_bh(&pn->l2tp_tunnel_idr_lock);
1514+
idr_replace(&pn->l2tp_tunnel_idr, tunnel, tunnel->tunnel_id);
1515+
spin_unlock_bh(&pn->l2tp_tunnel_idr_lock);
15161516

15171517
trace_register_tunnel(tunnel);
15181518

0 commit comments

Comments
 (0)