[PATCH net 0/2] sctp: fix the race condition in sctp_destroy_sock in a proper way

[PATCH net 0/2] sctp: fix the race condition in sctp_destroy_sock in a proper way

[Xin Long]

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

[PATCH net 1/2] Revert "net/sctp: fix race condition in sctp_destroy_sock"

[Xin Long]

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

[PATCH net 2/2] sctp: delay auto_asconf init until binding the first addr

[Xin Long]

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

Re: [PATCH net 0/2] sctp: fix the race condition in sctp_destroy_sock in a proper way

[patchwork-bot+netdevbpf]

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

Re: [PATCH net 0/2] sctp: fix the race condition in sctp_destroy_sock in a proper way

[Salvatore Bonaccorso]

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

Re: [PATCH net 0/2] sctp: fix the race condition in sctp_destroy_sock in a proper way

[Xin Long]

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.