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: <CANn89i+scj506yqh4YYAf5PQspbpqxym+YDiaXZTPaODM4_ANA@mail.gmail.com>
Date:   Tue, 17 Jan 2023 09:08:23 +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 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.



> +               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
>