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: <c4685015-ee4f-598c-0e2f-70e0737f4685@I-love.SAKURA.ne.jp>
Date:   Sat, 19 Nov 2022 19:08:38 +0900
From:   Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To:     Jakub Sitnicki <jakub@...udflare.com>,
        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 2022/11/19 2:50, Jakub Sitnicki wrote:
> 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:

Below is another approach I was thinking of, for reusing existing locks is prone
to locking bugs like [2] and [3].

I couldn't interpret "Write-protected by @sk_callback_lock." part because
it does not say what lock is needed for protecting sk_user_data for read access.

Is it possible to use a mutex dedicated for l2tp_tunnel_destruct() (and optionally
setup_udp_tunnel_sock_no_enable() in order not to create l2tp_tunnel_register_mutex =>
cpu_hotplug_lock chain) ?

By the way I haven't heard an response on

  Since userspace-supplied file descriptor has to be a datagram socket,
  can we somehow copy the source/destination addresses from
  userspace-supplied socket to kernel-created socket?

at https://lkml.kernel.org/r/c9695548-3f27-dda1-3124-ec21da106741@I-love.SAKURA.ne.jp
(that is, always create a new socket in order to be able to assign lockdep class
before that socket is used).

diff --git a/include/net/sock.h b/include/net/sock.h
index e0517ecc6531..49473013afa6 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -323,7 +323,7 @@ struct sk_filter;
   *	@sk_tskey: counter to disambiguate concurrent tstamp requests
   *	@sk_zckey: counter to order MSG_ZEROCOPY notifications
   *	@sk_socket: Identd and reporting IO signals
-  *	@sk_user_data: RPC layer private data. Write-protected by @sk_callback_lock.
+  *	@sk_user_data: RPC layer private data.
   *	@sk_frag: cached page frag
   *	@sk_peek_off: current peek_offset value
   *	@sk_send_head: front of stuff to transmit
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 754fdda8a5f5..2bfcf6968d89 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1150,10 +1150,8 @@ static void l2tp_tunnel_destruct(struct sock *sk)
 	}
 
 	/* Remove hooks into tunnel socket */
-	write_lock_bh(&sk->sk_callback_lock);
 	sk->sk_destruct = tunnel->old_sk_destruct;
 	sk->sk_user_data = NULL;
-	write_unlock_bh(&sk->sk_callback_lock);
 
 	/* Call the original destructor */
 	if (sk->sk_destruct)
@@ -1455,6 +1453,7 @@ static int l2tp_validate_socket(const struct sock *sk, const struct net *net,
 int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
 			 struct l2tp_tunnel_cfg *cfg)
 {
+	static DEFINE_MUTEX(l2tp_tunnel_register_mutex);
 	struct l2tp_tunnel *tunnel_walk;
 	struct l2tp_net *pn;
 	struct socket *sock;
@@ -1474,7 +1473,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
 	}
 
 	sk = sock->sk;
-	write_lock(&sk->sk_callback_lock);
+	mutex_lock(&l2tp_tunnel_register_mutex);
 
 	ret = l2tp_validate_socket(sk, net, tunnel->encap);
 	if (ret < 0)
@@ -1519,19 +1518,18 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
 
 	trace_register_tunnel(tunnel);
 
+	mutex_unlock(&l2tp_tunnel_register_mutex);
 	if (tunnel->fd >= 0)
 		sockfd_put(sock);
 
-	write_unlock(&sk->sk_callback_lock);
 	return 0;
 
 err_sock:
+	mutex_unlock(&l2tp_tunnel_register_mutex);
 	if (tunnel->fd < 0)
 		sock_release(sock);
 	else
 		sockfd_put(sock);
-
-	write_unlock(&sk->sk_callback_lock);
 err:
 	return ret;
 }