netdev - Re: [PATCH 6.1-rc6] l2tp: call udp_tunnel_encap_enable() and sock_release() without sk_callback

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: <87zgconn3g.fsf@cloudflare.com>
Date:   Fri, 18 Nov 2022 18:50:43 +0100
From:   Jakub Sitnicki <jakub@...udflare.com>
To:     Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        Eric Dumazet <edumazet@...gle.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        David Ahern <dsahern@...nel.org>,
        Tom Parkin <tparkin@...alix.com>,
        syzbot <syzbot+703d9e154b3b58277261@...kaller.appspotmail.com>,
        netdev@...r.kernel.org, syzkaller-bugs@...glegroups.com,
        Haowei Yan <g1042620637@...il.com>
Subject: Re: [PATCH 6.1-rc6] l2tp: call udp_tunnel_encap_enable() and
 sock_release() without sk_callback_lock

On Fri, Nov 18, 2022 at 04:36 AM -08, Eric Dumazet wrote:
> On Fri, Nov 18, 2022 at 3:51 AM Tetsuo Handa
> <penguin-kernel@...ove.sakura.ne.jp> wrote:
>>
>> syzbot is reporting sleep in atomic context at l2tp_tunnel_register() [1],
>> for commit b68777d54fac ("l2tp: Serialize access to sk_user_data with
>> sk_callback_lock") missed that udp_tunnel_encap_enable() from
>> setup_udp_tunnel_sock() might sleep.
>>
>> Since we don't want to drop sk->sk_callback_lock inside
>> setup_udp_tunnel_sock() right before calling udp_tunnel_encap_enable(),
>> introduce a variant which does not call udp_tunnel_encap_enable(). And
>> call udp_tunnel_encap_enable() after dropping sk->sk_callback_lock.
>>
>> Also, drop sk->sk_callback_lock before calling sock_release() in order to
>> avoid circular locking dependency problem.
>
> Please look at recent discussion, your patch does not address another
> fundamental problem.
>
> Also, Jakub was working on a fix already. Perhaps sync with him to
> avoid duplicate work.
>
> https://lore.kernel.org/netdev/20221114191619.124659-1-jakub@cloudflare.com/T/
>
> Thanks.

Thanks for the patch, Tetsuo.

As Eric has pointed out [1], there is another problem - in addition to
sleeping in atomic context, I have also failed to use the write_lock
variant which disabled BH locally.

The latter bug can lead to dead-locks, as reported by syzcaller [2, 3],
because we grab sk_callback_lock in softirq context, which can then
block waiting on us if:

1) it runs on the same CPU, or

       CPU0
       ----
  lock(clock-AF_INET6);
  <Interrupt>
    lock(clock-AF_INET6);

2) lock ordering leads to priority inversion

       CPU0                    CPU1
       ----                    ----
  lock(clock-AF_INET6);
                               local_irq_disable();
                               lock(&tcp_hashinfo.bhash[i].lock);
                               lock(clock-AF_INET6);
  <Interrupt>
    lock(&tcp_hashinfo.bhash[i].lock);

IOW, your patch works if we also s/write_\(un\)\?lock/write_\1lock_bh/.

But, I also have an alternative idea - instead of pulling the function
call that might sleep out of the critical section, I think we can make
the critical section much shorter by rearranging the tunnel
initialization code slightly. That is, a change like below.

-jkbs

[1] https://lore.kernel.org/netdev/CANn89iLQUZnyGNCn2GpW31FXpE_Lt7a5Urr21RqzfAE4sYxs+w@mail.gmail.com/
[2] https://lore.kernel.org/netdev/000000000000e38b6605eda76f98@google.com
[3] https://lore.kernel.org/netdev/000000000000dfa31e05eda76f75@google.com/


--8<--

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 754fdda8a5f5..07454c0418e3 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1474,11 +1474,15 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
 	}
 
 	sk = sock->sk;
-	write_lock(&sk->sk_callback_lock);
+	write_lock_bh(&sk->sk_callback_lock);
 
 	ret = l2tp_validate_socket(sk, net, tunnel->encap);
 	if (ret < 0)
 		goto err_sock;
+	if (tunnel->encap != L2TP_ENCAPTYPE_UDP)
+		rcu_assign_sk_user_data(sk, tunnel);
+
+	write_unlock_bh(&sk->sk_callback_lock);
 
 	tunnel->l2tp_net = net;
 	pn = l2tp_pernet(net);
@@ -1507,8 +1511,6 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
 		};
 
 		setup_udp_tunnel_sock(net, sock, &udp_cfg);
-	} else {
-		rcu_assign_sk_user_data(sk, tunnel);
 	}
 
 	tunnel->old_sk_destruct = sk->sk_destruct;
@@ -1522,7 +1524,6 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
 	if (tunnel->fd >= 0)
 		sockfd_put(sock);
 
-	write_unlock(&sk->sk_callback_lock);
 	return 0;
 
 err_sock:
@@ -1530,8 +1531,6 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
 		sock_release(sock);
 	else
 		sockfd_put(sock);
-
-	write_unlock(&sk->sk_callback_lock);
 err:
 	return ret;
 }