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] [day] [month] [year] [list]

Message-ID: <Y8Z+/mH8Jv2Y3M6V@debian>
Date:   Tue, 17 Jan 2023 11:57:02 +0100
From:   Guillaume Nault <gnault@...hat.com>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     Cong Wang <xiyou.wangcong@...il.com>, netdev@...r.kernel.org,
        saeed@...nel.org, 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 09:10:20AM +0100, Eric Dumazet wrote:
> 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:
> > >  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!