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 |
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>
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 >
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);
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 --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);