From owner-svn-src-all@freebsd.org Fri Jun 2 09:54:43 2017 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 7F589BF39C2; Fri, 2 Jun 2017 09:54:43 +0000 (UTC) (envelope-from ae@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 43473817C9; Fri, 2 Jun 2017 09:54:43 +0000 (UTC) (envelope-from ae@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id v529sgUN054425; Fri, 2 Jun 2017 09:54:42 GMT (envelope-from ae@FreeBSD.org) Received: (from ae@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id v529sgcB054420; Fri, 2 Jun 2017 09:54:42 GMT (envelope-from ae@FreeBSD.org) Message-Id: <201706020954.v529sgcB054420@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: ae set sender to ae@FreeBSD.org using -f From: "Andrey V. Elsukov" Date: Fri, 2 Jun 2017 09:54:42 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-11@freebsd.org Subject: svn commit: r319492 - stable/11/sys/netipsec X-SVN-Group: stable-11 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 02 Jun 2017 09:54:43 -0000 Author: ae Date: Fri Jun 2 09:54:41 2017 New Revision: 319492 URL: https://svnweb.freebsd.org/changeset/base/319492 Log: MFC r318734: Fix possible double releasing for SA reference. There are two possible ways how crypto callback are called: directly from caller and deffered from crypto thread. For inbound packets the direct call chain is the following: IPSEC_INPUT() method -> ipsec_common_input() -> xform_input() -> -> crypto_dispatch() -> crypto_invoke() -> crypto_done() -> -> xform_input_cb() -> ipsec[46]_common_input_cb() -> netisr_queue(). The SA reference is held while crypto processing is not finished. The error handling code wrongly expected that crypto callback always called from the crypto thread context, and it did SA reference releasing in xform_input_cb(). But when the crypto callback called directly, in case of error (e.g. data authentification failed) the error handling in ipsec_common_input() also did SA reference releasing. To fix this, remove error handling from ipsec_common_input() and do it in xform_input() before crypto_dispatch(). PR: 219356 MFC r318738: Fix possible double releasing for SA and SP references. There are two possible ways how crypto callback are called: directly from caller and deffered from crypto thread. For outbound packets the direct call chain is the following: IPSEC_OUTPUT() method -> ipsec[46]_common_output() -> -> ipsec[46]_perform_request() -> xform_output() -> -> crypto_dispatch() -> crypto_invoke() -> crypto_done() -> -> xform_output_cb() -> ipsec_process_done() -> ip[6]_output(). The SA and SP references are held while crypto processing is not finished. The error handling code wrongly expected that crypto callback always called from the crypto thread context, and it did references releasing in xform_output_cb(). But when the crypto callback called directly, in case of error the error handling code in ipsec[46]_perform_request() also did references releasing. To fix this, remove error handling from ipsec[46]_perform_request() and do it in xform_output() before crypto_dispatch(). Approved by: re (kib) Modified: stable/11/sys/netipsec/ipsec_input.c stable/11/sys/netipsec/ipsec_output.c stable/11/sys/netipsec/xform_ah.c stable/11/sys/netipsec/xform_esp.c stable/11/sys/netipsec/xform_ipcomp.c Directory Properties: stable/11/ (props changed) Modified: stable/11/sys/netipsec/ipsec_input.c ============================================================================== --- stable/11/sys/netipsec/ipsec_input.c Fri Jun 2 07:03:31 2017 (r319491) +++ stable/11/sys/netipsec/ipsec_input.c Fri Jun 2 09:54:41 2017 (r319492) @@ -223,8 +223,6 @@ ipsec_common_input(struct mbuf *m, int skip, int proto * everything else. */ error = (*sav->tdb_xform->xf_input)(m, sav, skip, protoff); - if (error != 0) - key_freesav(&sav); return (error); } Modified: stable/11/sys/netipsec/ipsec_output.c ============================================================================== --- stable/11/sys/netipsec/ipsec_output.c Fri Jun 2 07:03:31 2017 (r319491) +++ stable/11/sys/netipsec/ipsec_output.c Fri Jun 2 09:54:41 2017 (r319492) @@ -273,10 +273,6 @@ ipsec4_perform_request(struct mbuf *m, struct secpolic goto bad; } error = (*sav->tdb_xform->xf_output)(m, sp, sav, idx, i, off); - if (error != 0) { - key_freesav(&sav); - key_freesp(&sp); - } return (error); bad: IPSECSTAT_INC(ips_out_inval); @@ -581,10 +577,6 @@ ipsec6_perform_request(struct mbuf *m, struct secpolic goto bad; } error = (*sav->tdb_xform->xf_output)(m, sp, sav, idx, i, off); - if (error != 0) { - key_freesav(&sav); - key_freesp(&sp); - } return (error); bad: IPSEC6STAT_INC(ips_out_inval); Modified: stable/11/sys/netipsec/xform_ah.c ============================================================================== --- stable/11/sys/netipsec/xform_ah.c Fri Jun 2 07:03:31 2017 (r319491) +++ stable/11/sys/netipsec/xform_ah.c Fri Jun 2 09:54:41 2017 (r319492) @@ -566,8 +566,8 @@ ah_input(struct mbuf *m, struct secasvar *sav, int ski if (ah == NULL) { DPRINTF(("ah_input: cannot pullup header\n")); AHSTAT_INC(ahs_hdrops); /*XXX*/ - m_freem(m); - return ENOBUFS; + error = ENOBUFS; + goto bad; } /* Check replay window, if applicable. */ @@ -578,8 +578,8 @@ ah_input(struct mbuf *m, struct secasvar *sav, int ski AHSTAT_INC(ahs_replay); DPRINTF(("%s: packet replay failure: %s\n", __func__, ipsec_sa2str(sav, buf, sizeof(buf)))); - m_freem(m); - return (EACCES); + error = EACCES; + goto bad; } cryptoid = sav->tdb_cryptoid; SECASVAR_UNLOCK(sav); @@ -595,8 +595,8 @@ ah_input(struct mbuf *m, struct secasvar *sav, int ski ipsec_address(&sav->sah->saidx.dst, buf, sizeof(buf)), (u_long) ntohl(sav->spi))); AHSTAT_INC(ahs_badauthl); - m_freem(m); - return EACCES; + error = EACCES; + goto bad; } AHSTAT_ADD(ahs_ibytes, m->m_pkthdr.len - skip - hl); @@ -606,8 +606,8 @@ ah_input(struct mbuf *m, struct secasvar *sav, int ski DPRINTF(("%s: failed to acquire crypto descriptor\n", __func__)); AHSTAT_INC(ahs_crypto); - m_freem(m); - return ENOBUFS; + error = ENOBUFS; + goto bad; } crda = crp->crp_desc; @@ -629,8 +629,8 @@ ah_input(struct mbuf *m, struct secasvar *sav, int ski DPRINTF(("%s: failed to allocate xform_data\n", __func__)); AHSTAT_INC(ahs_crypto); crypto_freereq(crp); - m_freem(m); - return ENOBUFS; + error = ENOBUFS; + goto bad; } /* @@ -650,6 +650,7 @@ ah_input(struct mbuf *m, struct secasvar *sav, int ski AHSTAT_INC(ahs_hdrops); free(xd, M_XDATA); crypto_freereq(crp); + key_freesav(&sav); return (error); } @@ -668,6 +669,10 @@ ah_input(struct mbuf *m, struct secasvar *sav, int ski xd->skip = skip; xd->cryptoid = cryptoid; return (crypto_dispatch(crp)); +bad: + m_freem(m); + key_freesav(&sav); + return (error); } /* @@ -1044,6 +1049,8 @@ ah_output(struct mbuf *m, struct secpolicy *sp, struct bad: if (m) m_freem(m); + key_freesav(&sav); + key_freesp(&sp); return (error); } Modified: stable/11/sys/netipsec/xform_esp.c ============================================================================== --- stable/11/sys/netipsec/xform_esp.c Fri Jun 2 07:03:31 2017 (r319491) +++ stable/11/sys/netipsec/xform_esp.c Fri Jun 2 09:54:41 2017 (r319492) @@ -272,18 +272,18 @@ esp_input(struct mbuf *m, struct secasvar *sav, int sk struct newesp *esp; uint8_t *ivp; uint64_t cryptoid; - int plen, alen, hlen; + int alen, error, hlen, plen; IPSEC_ASSERT(sav != NULL, ("null SA")); IPSEC_ASSERT(sav->tdb_encalgxform != NULL, ("null encoding xform")); + error = EINVAL; /* Valid IP Packet length ? */ if ( (skip&3) || (m->m_pkthdr.len&3) ){ DPRINTF(("%s: misaligned packet, skip %u pkt len %u", __func__, skip, m->m_pkthdr.len)); ESPSTAT_INC(esps_badilen); - m_freem(m); - return EINVAL; + goto bad; } /* XXX don't pullup, just copy header */ IP6_EXTHDR_GET(esp, struct newesp *, m, skip, sizeof (struct newesp)); @@ -314,8 +314,7 @@ esp_input(struct mbuf *m, struct secasvar *sav, int sk ipsec_address(&sav->sah->saidx.dst, buf, sizeof(buf)), (u_long)ntohl(sav->spi))); ESPSTAT_INC(esps_badilen); - m_freem(m); - return EINVAL; + goto bad; } /* @@ -328,8 +327,8 @@ esp_input(struct mbuf *m, struct secasvar *sav, int sk DPRINTF(("%s: packet replay check for %s\n", __func__, ipsec_sa2str(sav, buf, sizeof(buf)))); ESPSTAT_INC(esps_replay); - m_freem(m); - return (EACCES); + error = EACCES; + goto bad; } } cryptoid = sav->tdb_cryptoid; @@ -344,8 +343,8 @@ esp_input(struct mbuf *m, struct secasvar *sav, int sk DPRINTF(("%s: failed to acquire crypto descriptors\n", __func__)); ESPSTAT_INC(esps_crypto); - m_freem(m); - return ENOBUFS; + error = ENOBUFS; + goto bad; } /* Get IPsec-specific opaque pointer */ @@ -354,8 +353,8 @@ esp_input(struct mbuf *m, struct secasvar *sav, int sk DPRINTF(("%s: failed to allocate xform_data\n", __func__)); ESPSTAT_INC(esps_crypto); crypto_freereq(crp); - m_freem(m); - return ENOBUFS; + error = ENOBUFS; + goto bad; } if (esph != NULL) { @@ -425,6 +424,10 @@ esp_input(struct mbuf *m, struct secasvar *sav, int sk crde->crd_alg = espx->type; return (crypto_dispatch(crp)); +bad: + m_freem(m); + key_freesav(&sav); + return (error); } /* @@ -858,6 +861,8 @@ esp_output(struct mbuf *m, struct secpolicy *sp, struc bad: if (m) m_freem(m); + key_freesav(&sav); + key_freesp(&sp); return (error); } /* Modified: stable/11/sys/netipsec/xform_ipcomp.c ============================================================================== --- stable/11/sys/netipsec/xform_ipcomp.c Fri Jun 2 07:03:31 2017 (r319491) +++ stable/11/sys/netipsec/xform_ipcomp.c Fri Jun 2 09:54:41 2017 (r319492) @@ -194,34 +194,35 @@ ipcomp_input(struct mbuf *m, struct secasvar *sav, int struct cryptop *crp; struct ipcomp *ipcomp; caddr_t addr; - int hlen = IPCOMP_HLENGTH; + int error, hlen = IPCOMP_HLENGTH; /* * Check that the next header of the IPComp is not IPComp again, before * doing any real work. Given it is not possible to do double * compression it means someone is playing tricks on us. */ + error = ENOBUFS; if (m->m_len < skip + hlen && (m = m_pullup(m, skip + hlen)) == NULL) { IPCOMPSTAT_INC(ipcomps_hdrops); /*XXX*/ DPRINTF(("%s: m_pullup failed\n", __func__)); - return (ENOBUFS); + key_freesav(&sav); + return (error); } addr = (caddr_t) mtod(m, struct ip *) + skip; ipcomp = (struct ipcomp *)addr; if (ipcomp->comp_nxt == IPPROTO_IPCOMP) { - m_freem(m); IPCOMPSTAT_INC(ipcomps_pdrops); /* XXX have our own stats? */ DPRINTF(("%s: recursive compression detected\n", __func__)); - return (EINVAL); + error = EINVAL; + goto bad; } /* Get crypto descriptors */ crp = crypto_getreq(1); if (crp == NULL) { - m_freem(m); DPRINTF(("%s: no crypto descriptors\n", __func__)); IPCOMPSTAT_INC(ipcomps_crypto); - return ENOBUFS; + goto bad; } /* Get IPsec-specific opaque pointer */ xd = malloc(sizeof(*xd), M_XDATA, M_NOWAIT | M_ZERO); @@ -229,8 +230,7 @@ ipcomp_input(struct mbuf *m, struct secasvar *sav, int DPRINTF(("%s: cannot allocate xform_data\n", __func__)); IPCOMPSTAT_INC(ipcomps_crypto); crypto_freereq(crp); - m_freem(m); - return ENOBUFS; + goto bad; } crdc = crp->crp_desc; @@ -259,6 +259,10 @@ ipcomp_input(struct mbuf *m, struct secasvar *sav, int SECASVAR_UNLOCK(sav); return crypto_dispatch(crp); +bad: + m_freem(m); + key_freesav(&sav); + return (error); } /* @@ -506,6 +510,8 @@ ipcomp_output(struct mbuf *m, struct secpolicy *sp, st bad: if (m) m_freem(m); + key_freesav(&sav); + key_freesp(&sp); return (error); }