netdev - Re: [Patch net v3 2/2] l2tp: close all race conditions in l2tp_tunnel

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89iJ1FZvtyHgXC69YEVirg0B5moKM1eDLSU1skLQV0PNrGw@mail.gmail.com>
Date:   Tue, 17 Jan 2023 09:10:20 +0100
From:   Eric Dumazet <edumazet@...gle.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     netdev@...r.kernel.org, saeed@...nel.org, gnault@...hat.com,
        tparkin@...alix.com, Cong Wang <cong.wang@...edance.com>,
        syzbot+52866e24647f9a23403f@...kaller.appspotmail.com,
        syzbot+94cc2a66fc228b23f360@...kaller.appspotmail.com,
        Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        Jakub Sitnicki <jakub@...udflare.com>
Subject: Re: [Patch net v3 2/2] l2tp: close all race conditions in l2tp_tunnel_register()

On Tue, Jan 17, 2023 at 9:08 AM Eric Dumazet <edumazet@...gle.com> wrote:
>
> On Sat, Jan 14, 2023 at 4:01 AM Cong Wang <xiyou.wangcong@...il.com> wrote:
> >
> > From: Cong Wang <cong.wang@...edance.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@...kaller.appspotmail.com
> > Reported-by: syzbot+94cc2a66fc228b23f360@...kaller.appspotmail.com
> > Reported-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
> > Cc: Guillaume Nault <gnault@...hat.com>
> > Cc: Jakub Sitnicki <jakub@...udflare.com>
> > Cc: Eric Dumazet <edumazet@...gle.com>
> > Cc: Tom Parkin <tparkin@...alix.com>
> > Signed-off-by: Cong Wang <cong.wang@...edance.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);