Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH net 0/2] sctp: fix the race condition in sctp_destroy_sock in a proper way
@ 2021-05-02 21:11 Xin Long
  2021-05-02 21:11 ` [PATCH net 1/2] Revert "net/sctp: fix race condition in sctp_destroy_sock" Xin Long
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Xin Long @ 2021-05-02 21:11 UTC (permalink / raw)
  To: network dev, linux-sctp; +Cc: davem, kuba, Marcelo Ricardo Leitner, Or Cohen

The original fix introduced a dead lock, and has to be removed in
Patch 1/2, and we will get a proper way to fix it in Patch 2/2.

Xin Long (2):
  Revert "net/sctp: fix race condition in sctp_destroy_sock"
  sctp: delay auto_asconf init until binding the first addr

 net/sctp/socket.c | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

-- 
2.1.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH net 1/2] Revert "net/sctp: fix race condition in sctp_destroy_sock"
  2021-05-02 21:11 [PATCH net 0/2] sctp: fix the race condition in sctp_destroy_sock in a proper way Xin Long
@ 2021-05-02 21:11 ` Xin Long
  2021-05-02 21:11   ` [PATCH net 2/2] sctp: delay auto_asconf init until binding the first addr Xin Long
  2021-05-03 20:40 ` [PATCH net 0/2] sctp: fix the race condition in sctp_destroy_sock in a proper way patchwork-bot+netdevbpf
  2021-05-10 19:10 ` Salvatore Bonaccorso
  2 siblings, 1 reply; 6+ messages in thread
From: Xin Long @ 2021-05-02 21:11 UTC (permalink / raw)
  To: network dev, linux-sctp; +Cc: davem, kuba, Marcelo Ricardo Leitner, Or Cohen

This reverts commit b166a20b07382b8bc1dcee2a448715c9c2c81b5b.

This one has to be reverted as it introduced a dead lock, as
syzbot reported:

       CPU0                    CPU1
       ----                    ----
  lock(&net->sctp.addr_wq_lock);
                               lock(slock-AF_INET6);
                               lock(&net->sctp.addr_wq_lock);
  lock(slock-AF_INET6);

CPU0 is the thread of sctp_addr_wq_timeout_handler(), and CPU1
is that of sctp_close().

The original issue this commit fixed will be fixed in the next
patch.

Reported-by: syzbot+959223586843e69a2674@syzkaller.appspotmail.com
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/socket.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index b7b9013..76a388b5 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -1520,9 +1520,11 @@ static void sctp_close(struct sock *sk, long timeout)
 
 	/* Supposedly, no process has access to the socket, but
 	 * the net layers still may.
+	 * Also, sctp_destroy_sock() needs to be called with addr_wq_lock
+	 * held and that should be grabbed before socket lock.
 	 */
-	local_bh_disable();
-	bh_lock_sock(sk);
+	spin_lock_bh(&net->sctp.addr_wq_lock);
+	bh_lock_sock_nested(sk);
 
 	/* Hold the sock, since sk_common_release() will put sock_put()
 	 * and we have just a little more cleanup.
@@ -1531,7 +1533,7 @@ static void sctp_close(struct sock *sk, long timeout)
 	sk_common_release(sk);
 
 	bh_unlock_sock(sk);
-	local_bh_enable();
+	spin_unlock_bh(&net->sctp.addr_wq_lock);
 
 	sock_put(sk);
 
@@ -4991,6 +4993,9 @@ static int sctp_init_sock(struct sock *sk)
 	sk_sockets_allocated_inc(sk);
 	sock_prot_inuse_add(net, sk->sk_prot, 1);
 
+	/* Nothing can fail after this block, otherwise
+	 * sctp_destroy_sock() will be called without addr_wq_lock held
+	 */
 	if (net->sctp.default_auto_asconf) {
 		spin_lock(&sock_net(sk)->sctp.addr_wq_lock);
 		list_add_tail(&sp->auto_asconf_list,
@@ -5025,9 +5030,7 @@ static void sctp_destroy_sock(struct sock *sk)
 
 	if (sp->do_auto_asconf) {
 		sp->do_auto_asconf = 0;
-		spin_lock_bh(&sock_net(sk)->sctp.addr_wq_lock);
 		list_del(&sp->auto_asconf_list);
-		spin_unlock_bh(&sock_net(sk)->sctp.addr_wq_lock);
 	}
 	sctp_endpoint_free(sp->ep);
 	local_bh_disable();
-- 
2.1.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH net 2/2] sctp: delay auto_asconf init until binding the first addr
  2021-05-02 21:11 ` [PATCH net 1/2] Revert "net/sctp: fix race condition in sctp_destroy_sock" Xin Long
@ 2021-05-02 21:11   ` Xin Long
  0 siblings, 0 replies; 6+ messages in thread
From: Xin Long @ 2021-05-02 21:11 UTC (permalink / raw)
  To: network dev, linux-sctp; +Cc: davem, kuba, Marcelo Ricardo Leitner, Or Cohen

As Or Cohen described:

  If sctp_destroy_sock is called without sock_net(sk)->sctp.addr_wq_lock
  held and sp->do_auto_asconf is true, then an element is removed
  from the auto_asconf_splist without any proper locking.

  This can happen in the following functions:
  1. In sctp_accept, if sctp_sock_migrate fails.
  2. In inet_create or inet6_create, if there is a bpf program
     attached to BPF_CGROUP_INET_SOCK_CREATE which denies
     creation of the sctp socket.

This patch is to fix it by moving the auto_asconf init out of
sctp_init_sock(), by which inet_create()/inet6_create() won't
need to operate it in sctp_destroy_sock() when calling
sk_common_release().

It also makes more sense to do auto_asconf init while binding the
first addr, as auto_asconf actually requires an ANY addr bind,
see it in sctp_addr_wq_timeout_handler().

This addresses CVE-2021-23133.

Fixes: 610236587600 ("bpf: Add new cgroup attach type to enable sock modifications")
Reported-by: Or Cohen <orcohen@paloaltonetworks.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/socket.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 76a388b5..40f9f6c 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -357,6 +357,18 @@ static struct sctp_af *sctp_sockaddr_af(struct sctp_sock *opt,
 	return af;
 }
 
+static void sctp_auto_asconf_init(struct sctp_sock *sp)
+{
+	struct net *net = sock_net(&sp->inet.sk);
+
+	if (net->sctp.default_auto_asconf) {
+		spin_lock(&net->sctp.addr_wq_lock);
+		list_add_tail(&sp->auto_asconf_list, &net->sctp.auto_asconf_splist);
+		spin_unlock(&net->sctp.addr_wq_lock);
+		sp->do_auto_asconf = 1;
+	}
+}
+
 /* Bind a local address either to an endpoint or to an association.  */
 static int sctp_do_bind(struct sock *sk, union sctp_addr *addr, int len)
 {
@@ -418,8 +430,10 @@ static int sctp_do_bind(struct sock *sk, union sctp_addr *addr, int len)
 		return -EADDRINUSE;
 
 	/* Refresh ephemeral port.  */
-	if (!bp->port)
+	if (!bp->port) {
 		bp->port = inet_sk(sk)->inet_num;
+		sctp_auto_asconf_init(sp);
+	}
 
 	/* Add the address to the bind address list.
 	 * Use GFP_ATOMIC since BHs will be disabled.
@@ -4993,19 +5007,6 @@ static int sctp_init_sock(struct sock *sk)
 	sk_sockets_allocated_inc(sk);
 	sock_prot_inuse_add(net, sk->sk_prot, 1);
 
-	/* Nothing can fail after this block, otherwise
-	 * sctp_destroy_sock() will be called without addr_wq_lock held
-	 */
-	if (net->sctp.default_auto_asconf) {
-		spin_lock(&sock_net(sk)->sctp.addr_wq_lock);
-		list_add_tail(&sp->auto_asconf_list,
-		    &net->sctp.auto_asconf_splist);
-		sp->do_auto_asconf = 1;
-		spin_unlock(&sock_net(sk)->sctp.addr_wq_lock);
-	} else {
-		sp->do_auto_asconf = 0;
-	}
-
 	local_bh_enable();
 
 	return 0;
@@ -9401,6 +9402,8 @@ static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
 			return err;
 	}
 
+	sctp_auto_asconf_init(newsp);
+
 	/* Move any messages in the old socket's receive queue that are for the
 	 * peeled off association to the new socket's receive queue.
 	 */
-- 
2.1.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net 0/2] sctp: fix the race condition in sctp_destroy_sock in a proper way
  2021-05-02 21:11 [PATCH net 0/2] sctp: fix the race condition in sctp_destroy_sock in a proper way Xin Long
  2021-05-02 21:11 ` [PATCH net 1/2] Revert "net/sctp: fix race condition in sctp_destroy_sock" Xin Long
@ 2021-05-03 20:40 ` patchwork-bot+netdevbpf
  2021-05-10 19:10 ` Salvatore Bonaccorso
  2 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-05-03 20:40 UTC (permalink / raw)
  To: Xin Long; +Cc: netdev, linux-sctp, davem, kuba, marcelo.leitner, orcohen

Hello:

This series was applied to netdev/net.git (refs/heads/master):

On Mon,  3 May 2021 05:11:40 +0800 you wrote:
> The original fix introduced a dead lock, and has to be removed in
> Patch 1/2, and we will get a proper way to fix it in Patch 2/2.
> 
> Xin Long (2):
>   Revert "net/sctp: fix race condition in sctp_destroy_sock"
>   sctp: delay auto_asconf init until binding the first addr
> 
> [...]

Here is the summary with links:
  - [net,1/2] Revert "net/sctp: fix race condition in sctp_destroy_sock"
    https://git.kernel.org/netdev/net/c/01bfe5e8e428
  - [net,2/2] sctp: delay auto_asconf init until binding the first addr
    https://git.kernel.org/netdev/net/c/34e5b0118685

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net 0/2] sctp: fix the race condition in sctp_destroy_sock in a proper way
  2021-05-02 21:11 [PATCH net 0/2] sctp: fix the race condition in sctp_destroy_sock in a proper way Xin Long
  2021-05-02 21:11 ` [PATCH net 1/2] Revert "net/sctp: fix race condition in sctp_destroy_sock" Xin Long
  2021-05-03 20:40 ` [PATCH net 0/2] sctp: fix the race condition in sctp_destroy_sock in a proper way patchwork-bot+netdevbpf
@ 2021-05-10 19:10 ` Salvatore Bonaccorso
  2021-05-10 19:18   ` Xin Long
  2 siblings, 1 reply; 6+ messages in thread
From: Salvatore Bonaccorso @ 2021-05-10 19:10 UTC (permalink / raw)
  To: Xin Long
  Cc: network dev, linux-sctp, davem, kuba, Marcelo Ricardo Leitner, Or Cohen

Hi Xin,

On Mon, May 03, 2021 at 05:11:40AM +0800, Xin Long wrote:
> The original fix introduced a dead lock, and has to be removed in
> Patch 1/2, and we will get a proper way to fix it in Patch 2/2.
> 
> Xin Long (2):
>   Revert "net/sctp: fix race condition in sctp_destroy_sock"
>   sctp: delay auto_asconf init until binding the first addr
> 
>  net/sctp/socket.c | 38 ++++++++++++++++++++++----------------
>  1 file changed, 22 insertions(+), 16 deletions(-)

The original commit which got reverted in this series, was already
applied to several stable series, namely all of 4.4.268, 4.9.268,
4.14.232, 4.19.189, 5.4.114, 5.10.32, 5.11.16.

Is it planned to do the revert and bugfix in those as well?

Regards,
Salvatore

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net 0/2] sctp: fix the race condition in sctp_destroy_sock in a proper way
  2021-05-10 19:10 ` Salvatore Bonaccorso
@ 2021-05-10 19:18   ` Xin Long
  0 siblings, 0 replies; 6+ messages in thread
From: Xin Long @ 2021-05-10 19:18 UTC (permalink / raw)
  To: Salvatore Bonaccorso
  Cc: network dev, linux-sctp @ vger . kernel . org, davem,
	Jakub Kicinski, Marcelo Ricardo Leitner, Or Cohen

On Mon, May 10, 2021 at 3:10 PM Salvatore Bonaccorso <carnil@debian.org> wrote:
>
> Hi Xin,
>
> On Mon, May 03, 2021 at 05:11:40AM +0800, Xin Long wrote:
> > The original fix introduced a dead lock, and has to be removed in
> > Patch 1/2, and we will get a proper way to fix it in Patch 2/2.
> >
> > Xin Long (2):
> >   Revert "net/sctp: fix race condition in sctp_destroy_sock"
> >   sctp: delay auto_asconf init until binding the first addr
> >
> >  net/sctp/socket.c | 38 ++++++++++++++++++++++----------------
> >  1 file changed, 22 insertions(+), 16 deletions(-)
>
> The original commit which got reverted in this series, was already
> applied to several stable series, namely all of 4.4.268, 4.9.268,
> 4.14.232, 4.19.189, 5.4.114, 5.10.32, 5.11.16.
>
> Is it planned to do the revert and bugfix in those as well?
Yes. Thanks.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-02 21:11 [PATCH net 0/2] sctp: fix the race condition in sctp_destroy_sock in a proper way Xin Long
2021-05-02 21:11 ` [PATCH net 1/2] Revert "net/sctp: fix race condition in sctp_destroy_sock" Xin Long
2021-05-02 21:11   ` [PATCH net 2/2] sctp: delay auto_asconf init until binding the first addr Xin Long
2021-05-03 20:40 ` [PATCH net 0/2] sctp: fix the race condition in sctp_destroy_sock in a proper way patchwork-bot+netdevbpf
2021-05-10 19:10 ` Salvatore Bonaccorso
2021-05-10 19:18   ` Xin Long

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git