diff mbox series

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

Message ID 20230114030137.672706-3-xiyou.wangcong@gmail.com (mailing list archive)
State Accepted
Commit 0b2c59720e65885a394a017d0cf9cab118914682
Delegated to: Netdev Maintainers
Headers show
Series l2tp: fix race conditions in l2tp_tunnel_register() | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers fail 2 blamed authors not CCed: davem@davemloft.net g.nault@alphalink.fr; 4 maintainers not CCed: davem@davemloft.net pabeni@redhat.com g.nault@alphalink.fr kuba@kernel.org
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 59 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Cong Wang Jan. 14, 2023, 3:01 a.m. UTC 
From: Cong Wang <cong.wang@bytedance.com>

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: 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>
---
 net/l2tp/l2tp_core.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

Comments

Guillaume Nault Jan. 16, 2023, 11:08 a.m. UTC | #1 
On Fri, Jan 13, 2023 at 07:01:37PM -0800, Cong Wang wrote:
> From: Cong Wang <cong.wang@bytedance.com>
> 
> 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().

Reviewed-by: Guillaume Nault <gnault@redhat.com>
Eric Dumazet Jan. 17, 2023, 8:08 a.m. UTC | #2 
On Sat, Jan 14, 2023 at 4:01 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> From: Cong Wang <cong.wang@bytedance.com>
>
> 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: 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>
> ---
>  net/l2tp/l2tp_core.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index e9c0ce0b7972..b6554e32bb12 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;
> @@ -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)
>  {
> @@ -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) {

I think we need to write_unlock_bh(&sk->sk_callback_lock)
before release_sock(), or risk lockdep reports.



> +               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,
> @@ -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);
>
> --
> 2.34.1
>
Eric Dumazet Jan. 17, 2023, 8:10 a.m. UTC | #3 
On Tue, Jan 17, 2023 at 9:08 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Sat, Jan 14, 2023 at 4:01 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > From: Cong Wang <cong.wang@bytedance.com>
> >
> > 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: 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>
> > ---
> >  net/l2tp/l2tp_core.c | 28 ++++++++++++++--------------
> >  1 file changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> > index e9c0ce0b7972..b6554e32bb12 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;
> > @@ -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)
> >  {
> > @@ -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) {
>
> I think we need to write_unlock_bh(&sk->sk_callback_lock)
> before release_sock(), or risk lockdep reports.
>

Any objection if I propose :

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index b6554e32bb12ae7813cc06c01e4d1380af667375..03608d3ded4b83d1e59e064e482f54cffcdf5240
100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1483,10 +1483,8 @@ int l2tp_tunnel_register(struct l2tp_tunnel
*tunnel, struct net *net,
        lock_sock(sk);
        write_lock_bh(&sk->sk_callback_lock);
        ret = l2tp_validate_socket(sk, net, tunnel->encap);
-       if (ret < 0) {
-               release_sock(sk);
+       if (ret < 0)
                goto err_inval_sock;
-       }
        rcu_assign_sk_user_data(sk, tunnel);
        write_unlock_bh(&sk->sk_callback_lock);

@@ -1523,6 +1521,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel
*tunnel, struct net *net,

 err_inval_sock:
        write_unlock_bh(&sk->sk_callback_lock);
+       release_sock(sk);

        if (tunnel->fd < 0)
                sock_release(sock);
Guillaume Nault Jan. 17, 2023, 10:57 a.m. UTC | #4 
On Tue, Jan 17, 2023 at 09:10:20AM +0100, Eric Dumazet wrote:
> On Tue, Jan 17, 2023 at 9:08 AM Eric Dumazet <edumazet@google.com> wrote:
> > On Sat, Jan 14, 2023 at 4:01 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > >  int l2tp_tunnel_create(int fd, int version, u32 tunnel_id, u32 peer_tunnel_id,
> > >                        struct l2tp_tunnel_cfg *cfg, struct l2tp_tunnel **tunnelp)
> > >  {
> > > @@ -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) {
> >
> > I think we need to write_unlock_bh(&sk->sk_callback_lock)
> > before release_sock(), or risk lockdep reports.
> >
> 
> Any objection if I propose :
> 
> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index b6554e32bb12ae7813cc06c01e4d1380af667375..03608d3ded4b83d1e59e064e482f54cffcdf5240
> 100644
> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c
> @@ -1483,10 +1483,8 @@ int l2tp_tunnel_register(struct l2tp_tunnel
> *tunnel, struct net *net,
>         lock_sock(sk);
>         write_lock_bh(&sk->sk_callback_lock);
>         ret = l2tp_validate_socket(sk, net, tunnel->encap);
> -       if (ret < 0) {
> -               release_sock(sk);
> +       if (ret < 0)
>                 goto err_inval_sock;
> -       }
>         rcu_assign_sk_user_data(sk, tunnel);
>         write_unlock_bh(&sk->sk_callback_lock);
> 
> @@ -1523,6 +1521,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel
> *tunnel, struct net *net,
> 
>  err_inval_sock:
>         write_unlock_bh(&sk->sk_callback_lock);
> +       release_sock(sk);
> 
>         if (tunnel->fd < 0)
>                 sock_release(sock);

Indeed, that looks more correct.
Thanks!
diff mbox series

Patch

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index e9c0ce0b7972..b6554e32bb12 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;
@@ -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)
 {
@@ -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,
@@ -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);