From owner-dev-commits-src-branches@freebsd.org Tue Sep 7 13:36:29 2021 Return-Path: Delivered-To: dev-commits-src-branches@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id E0584664BBB; Tue, 7 Sep 2021 13:36:29 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4H3mWd0Zzyz3vvp; Tue, 7 Sep 2021 13:36:29 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id DBC44136BA; Tue, 7 Sep 2021 13:36:28 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.16.1/8.16.1) with ESMTP id 187DaSj7008142; Tue, 7 Sep 2021 13:36:28 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 187DaSk2008141; Tue, 7 Sep 2021 13:36:28 GMT (envelope-from git) Date: Tue, 7 Sep 2021 13:36:28 GMT Message-Id: <202109071336.187DaSk2008141@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org From: Mark Johnston Subject: git: 6053349c46a2 - stable/13 - sctp: Fix racy UNBOUND flag check in sctp_inpcb_bind() MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: markj X-Git-Repository: src X-Git-Refname: refs/heads/stable/13 X-Git-Reftype: branch X-Git-Commit: 6053349c46a28132f56c5cb047e2f6a053d653a6 Auto-Submitted: auto-generated X-BeenThere: dev-commits-src-branches@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commits to the stable branches of the FreeBSD src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 07 Sep 2021 13:36:30 -0000 The branch stable/13 has been updated by markj: URL: https://cgit.FreeBSD.org/src/commit/?id=6053349c46a28132f56c5cb047e2f6a053d653a6 commit 6053349c46a28132f56c5cb047e2f6a053d653a6 Author: Mark Johnston AuthorDate: 2021-08-31 11:43:47 +0000 Commit: Mark Johnston CommitDate: 2021-09-07 13:36:19 +0000 sctp: Fix racy UNBOUND flag check in sctp_inpcb_bind() SCTP needs to avoid binding a given socket twice. The check used to avoid this is racy since neither the inpcb lock nor the global info lock is held. Fix it by synchronizing using the global info lock. In particular, sctp_inpcb_bind() may drop the inpcb lock in some cases, but the info lock is sufficient to prevent double insertion into PCB hash tables. Reported by: syzbot+548a8560d959669d0e12@syzkaller.appspotmail.com Reviewed by: tuexen Sponsored by: The FreeBSD Foundation (cherry picked from commit 4a36122b1db1b255cf21d926b997d524e6782429) --- sys/netinet/sctp_pcb.c | 108 ++++++++++++++++++++++++------------------------- 1 file changed, 53 insertions(+), 55 deletions(-) diff --git a/sys/netinet/sctp_pcb.c b/sys/netinet/sctp_pcb.c index def6292456d4..12f2d5d7fb76 100644 --- a/sys/netinet/sctp_pcb.c +++ b/sys/netinet/sctp_pcb.c @@ -2818,6 +2818,7 @@ sctp_inpcb_bind(struct socket *so, struct sockaddr *addr, KASSERT(td != NULL, ("%s: null thread", __func__)); + error = 0; lport = 0; bindall = 1; inp = (struct sctp_inpcb *)so->so_pcb; @@ -2830,10 +2831,13 @@ sctp_inpcb_bind(struct socket *so, struct sockaddr *addr, SCTPDBG_ADDR(SCTP_DEBUG_PCB1, addr); } #endif + SCTP_INP_INFO_WLOCK(); + SCTP_INP_WLOCK(inp); if ((inp->sctp_flags & SCTP_PCB_FLAGS_UNBOUND) == 0) { + error = EINVAL; /* already did a bind, subsequent binds NOT allowed ! */ - SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, EINVAL); - return (EINVAL); + SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, error); + goto out; } if (addr != NULL) { switch (addr->sa_family) { @@ -2844,12 +2848,14 @@ sctp_inpcb_bind(struct socket *so, struct sockaddr *addr, /* IPV6_V6ONLY socket? */ if (SCTP_IPV6_V6ONLY(inp)) { - SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, EINVAL); - return (EINVAL); + error = EINVAL; + SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, error); + goto out; } if (addr->sa_len != sizeof(*sin)) { - SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, EINVAL); - return (EINVAL); + error = EINVAL; + SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, error); + goto out; } sin = (struct sockaddr_in *)addr; @@ -2861,7 +2867,7 @@ sctp_inpcb_bind(struct socket *so, struct sockaddr *addr, */ if ((error = prison_local_ip4(td->td_ucred, &sin->sin_addr)) != 0) { SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, error); - return (error); + goto out; } if (sin->sin_addr.s_addr != INADDR_ANY) { bindall = 0; @@ -2879,10 +2885,10 @@ sctp_inpcb_bind(struct socket *so, struct sockaddr *addr, struct sockaddr_in6 *sin6; sin6 = (struct sockaddr_in6 *)addr; - if (addr->sa_len != sizeof(*sin6)) { - SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, EINVAL); - return (EINVAL); + error = EINVAL; + SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, error); + goto out; } lport = sin6->sin6_port; /* @@ -2893,14 +2899,15 @@ sctp_inpcb_bind(struct socket *so, struct sockaddr *addr, if ((error = prison_local_ip6(td->td_ucred, &sin6->sin6_addr, (SCTP_IPV6_V6ONLY(inp) != 0))) != 0) { SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, error); - return (error); + goto out; } if (!IN6_IS_ADDR_UNSPECIFIED(&sin6->sin6_addr)) { bindall = 0; /* KAME hack: embed scopeid */ if (sa6_embedscope(sin6, MODULE_GLOBAL(ip6_use_defzone)) != 0) { - SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, EINVAL); - return (EINVAL); + error = EINVAL; + SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, error); + goto out; } } /* this must be cleared for ifa_ifwithaddr() */ @@ -2909,12 +2916,11 @@ sctp_inpcb_bind(struct socket *so, struct sockaddr *addr, } #endif default: - SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, EAFNOSUPPORT); - return (EAFNOSUPPORT); + error = EAFNOSUPPORT; + SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, error); + goto out; } } - SCTP_INP_INFO_WLOCK(); - SCTP_INP_WLOCK(inp); /* Setup a vrf_id to be the default for the non-bind-all case. */ vrf_id = inp->def_vrf_id; @@ -2930,9 +2936,7 @@ sctp_inpcb_bind(struct socket *so, struct sockaddr *addr, if (ntohs(lport) < IPPORT_RESERVED) { if ((error = priv_check(td, PRIV_NETINET_RESERVEDPORT)) != 0) { SCTP_INP_DECR_REF(inp); - SCTP_INP_WUNLOCK(inp); - SCTP_INP_INFO_WUNLOCK(); - return (error); + goto out; } } SCTP_INP_WUNLOCK(inp); @@ -2959,9 +2963,9 @@ sctp_inpcb_bind(struct socket *so, struct sockaddr *addr, goto continue_anyway; } SCTP_INP_DECR_REF(inp); - SCTP_INP_INFO_WUNLOCK(); - SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, EADDRINUSE); - return (EADDRINUSE); + error = EADDRINUSE; + SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, error); + goto out_inp_unlocked; } } else { inp_tmp = sctp_pcb_findep(addr, 0, 1, vrf_id); @@ -2985,9 +2989,9 @@ sctp_inpcb_bind(struct socket *so, struct sockaddr *addr, goto continue_anyway; } SCTP_INP_DECR_REF(inp); - SCTP_INP_INFO_WUNLOCK(); - SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, EADDRINUSE); - return (EADDRINUSE); + error = EADDRINUSE; + SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, error); + goto out_inp_unlocked; } } continue_anyway: @@ -3002,10 +3006,9 @@ continue_anyway: (sctp_is_feature_on(inp_tmp, SCTP_PCB_FLAGS_PORTREUSE))) { port_reuse_active = 1; } else { - SCTP_INP_WUNLOCK(inp); - SCTP_INP_INFO_WUNLOCK(); - SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, EADDRINUSE); - return (EADDRINUSE); + error = EADDRINUSE; + SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, error); + goto out; } } } @@ -3018,10 +3021,8 @@ continue_anyway: last = MODULE_GLOBAL(ipport_hilastauto); } else if (ip_inp->inp_flags & INP_LOWPORT) { if ((error = priv_check(td, PRIV_NETINET_RESERVEDPORT)) != 0) { - SCTP_INP_WUNLOCK(inp); - SCTP_INP_INFO_WUNLOCK(); SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, error); - return (error); + goto out; } first = MODULE_GLOBAL(ipport_lowfirstauto); last = MODULE_GLOBAL(ipport_lowlastauto); @@ -3045,10 +3046,9 @@ continue_anyway: break; } if (--count == 0) { - SCTP_INP_WUNLOCK(inp); - SCTP_INP_INFO_WUNLOCK(); - SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, EADDRINUSE); - return (EADDRINUSE); + error = EADDRINUSE; + SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, error); + goto out; } if (candidate == last) candidate = first; @@ -3062,10 +3062,9 @@ continue_anyway: * this really should not happen. The guy did a non-blocking * bind and then did a close at the same time. */ - SCTP_INP_WUNLOCK(inp); - SCTP_INP_INFO_WUNLOCK(); - SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, EINVAL); - return (EINVAL); + error = EINVAL; + SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, error); + goto out; } /* ok we look clear to give out this port, so lets setup the binding */ if (bindall) { @@ -3153,21 +3152,19 @@ continue_anyway: vrf_id, SCTP_ADDR_NOT_LOCKED); } if (ifa == NULL) { + error = EADDRNOTAVAIL; /* Can't find an interface with that address */ - SCTP_INP_WUNLOCK(inp); - SCTP_INP_INFO_WUNLOCK(); - SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, EADDRNOTAVAIL); - return (EADDRNOTAVAIL); + SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, error); + goto out; } #ifdef INET6 if (addr->sa_family == AF_INET6) { /* GAK, more FIXME IFA lock? */ if (ifa->localifa_flags & SCTP_ADDR_IFA_UNUSEABLE) { /* Can't bind a non-existent addr. */ - SCTP_INP_WUNLOCK(inp); - SCTP_INP_INFO_WUNLOCK(); - SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, EINVAL); - return (EINVAL); + error = EINVAL; + SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_PCB, error); + goto out; } } #endif @@ -3180,11 +3177,8 @@ continue_anyway: /* add this address to the endpoint list */ error = sctp_insert_laddr(&inp->sctp_addr_list, ifa, 0); - if (error != 0) { - SCTP_INP_WUNLOCK(inp); - SCTP_INP_INFO_WUNLOCK(); - return (error); - } + if (error != 0) + goto out; inp->laddr_count++; } /* find the bucket */ @@ -3203,10 +3197,14 @@ continue_anyway: inp->sctp_lport = lport; /* turn off just the unbound flag */ + KASSERT((inp->sctp_flags & SCTP_PCB_FLAGS_UNBOUND) != 0, + ("%s: inp %p is already bound", __func__, inp)); inp->sctp_flags &= ~SCTP_PCB_FLAGS_UNBOUND; +out: SCTP_INP_WUNLOCK(inp); +out_inp_unlocked: SCTP_INP_INFO_WUNLOCK(); - return (0); + return (error); } static void