* [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 related [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 related [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, other threads:[~2021-05-10 19:18 UTC | newest] 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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).