[net,2/2] l2tp: close all race conditions in l2tp_tunnel_register()

Commit Message

Cong Wang Jan. 5, 2023, 7:13 p.m. UTC

From: Cong Wang <[e-mail]>

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: 37159ef2c1ae ("l2tp: fix a lockdep splat")
Fixes: 6b9f34239b00 ("l2tp: fix races in tunnel creation")
Reported-by: [e-mail]
Reported-by: [e-mail]
Reported-by: Tetsuo Handa <[e-mail]>
Cc: Guillaume Nault <[e-mail]>
Cc: Jakub Sitnicki <[e-mail]>
Cc: Eric Dumazet <[e-mail]>
Signed-off-by: Cong Wang <[e-mail]>
---
 net/l2tp/l2tp_core.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

Comments

Saeed Mahameed Jan. 7, 2023, 6:41 p.m. UTC | #1

On 05 Jan 11:13, Cong Wang wrote:
>From: Cong Wang <[e-mail]>
>
>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: 37159ef2c1ae ("l2tp: fix a lockdep splat")
>Fixes: 6b9f34239b00 ("l2tp: fix races in tunnel creation")

This patch relies on the previous one which doesn't include any tags.
If you are interested in this making it to -stable then maybe you need to
add those tags to the previous commit ?

I am not really familiar with the issue and how socket locks should work
here, but code wise LGTM.

Reviewed-by: Saeed Mahameed <[e-mail]>


>Reported-by: [e-mail]
>Reported-by: [e-mail]
>Reported-by: Tetsuo Handa <[e-mail]>
>Cc: Guillaume Nault <[e-mail]>
>Cc: Jakub Sitnicki <[e-mail]>
>Cc: Eric Dumazet <[e-mail]>
>Signed-off-by: Cong Wang <[e-mail]>

Cong Wang Jan. 7, 2023, 7:52 p.m. UTC | #2

On Sat, Jan 07, 2023 at 10:41:41AM -0800, Saeed Mahameed wrote:
> On 05 Jan 11:13, Cong Wang wrote:
> > From: Cong Wang <[e-mail]>
> > 
> > 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: 37159ef2c1ae ("l2tp: fix a lockdep splat")
> > Fixes: 6b9f34239b00 ("l2tp: fix races in tunnel creation")
> 
> This patch relies on the previous one which doesn't include any tags.
> If you are interested in this making it to -stable then maybe you need to
> add those tags to the previous commit ?
> 

But technically patch 1/2 does not fix anything alone, this is why I
heisitate to add any Fixes tag to it.

Since this is a patchset, I think maintainers can easily figure out this
is a whole set.

Thanks.

Jakub Kicinski Jan. 10, 2023, 1:36 a.m. UTC | #3

On Sat, 7 Jan 2023 11:52:23 -0800 Cong Wang wrote:
> But technically patch 1/2 does not fix anything alone, this is why I
> heisitate to add any Fixes tag to it.
> 
> Since this is a patchset, I think maintainers can easily figure out this
> is a whole set.

Yup, afaik prep patches don't need any extra tags if the dependency is
pretty obvious (same series and fix does not apply without the prep).

Patch

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 570249a91c6c..3cb5cbfd4399 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -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;
@@ -1376,8 +1376,6 @@  static int l2tp_tunnel_sock_create(struct net *net,
 	return err;
 }
 
-static struct lock_class_key l2tp_socket_class;
-
 void l2tp_tunnel_remove(struct l2tp_tunnel *tunnel)
 {
 	struct l2tp_net *pn = l2tp_pernet(tunnel->l2tp_net);
@@ -1492,22 +1490,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);
 
-	pn = l2tp_pernet(net);
-
-	sock_hold(sk);
-	tunnel->sock = sk;
-
-	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,
@@ -1518,12 +1510,19 @@  int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
 
 		setup_udp_tunnel_sock(net, sock, &udp_cfg);
 	}
-
 	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);
+
+	pn = l2tp_pernet(net);
+
+	sock_hold(sk);
+	tunnel->sock = sk;
+
+	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);