From 5f7c516f216995249523ed43e57e3f91bd8f5ff0 Mon Sep 17 00:00:00 2001 From: "Andrey V. Elsukov" Date: Tue, 23 May 2017 09:01:48 +0000 Subject: [PATCH] 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 after: 10 days --- sys/netipsec/ipsec_input.c | 2 -- sys/netipsec/xform_ah.c | 25 +++++++++++++++---------- sys/netipsec/xform_esp.c | 25 ++++++++++++++----------- sys/netipsec/xform_ipcomp.c | 20 ++++++++++++-------- 4 files changed, 41 insertions(+), 31 deletions(-) diff --git a/sys/netipsec/ipsec_input.c b/sys/netipsec/ipsec_input.c index c4acafecc243..e253cc0a31d2 100644 --- a/sys/netipsec/ipsec_input.c +++ b/sys/netipsec/ipsec_input.c @@ -223,8 +223,6 @@ ipsec_common_input(struct mbuf *m, int skip, int protoff, int af, int sproto) * everything else. */ error = (*sav->tdb_xform->xf_input)(m, sav, skip, protoff); - if (error != 0) - key_freesav(&sav); return (error); } diff --git a/sys/netipsec/xform_ah.c b/sys/netipsec/xform_ah.c index e5f80311b96b..521ce3447d12 100644 --- a/sys/netipsec/xform_ah.c +++ b/sys/netipsec/xform_ah.c @@ -566,8 +566,8 @@ ah_input(struct mbuf *m, struct secasvar *sav, int skip, int protoff) 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 skip, int protoff) 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 skip, int protoff) 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 skip, int protoff) 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 skip, int protoff) 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 skip, int protoff) 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 skip, int protoff) xd->skip = skip; xd->cryptoid = cryptoid; return (crypto_dispatch(crp)); +bad: + m_freem(m); + key_freesav(&sav); + return (error); } /* diff --git a/sys/netipsec/xform_esp.c b/sys/netipsec/xform_esp.c index 3c5ca6b65268..bf6fc9cb1973 100644 --- a/sys/netipsec/xform_esp.c +++ b/sys/netipsec/xform_esp.c @@ -272,18 +272,18 @@ esp_input(struct mbuf *m, struct secasvar *sav, int skip, int protoff) 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 skip, int protoff) 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 skip, int protoff) 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 skip, int protoff) 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 skip, int protoff) 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 skip, int protoff) crde->crd_alg = espx->type; return (crypto_dispatch(crp)); +bad: + m_freem(m); + key_freesav(&sav); + return (error); } /* diff --git a/sys/netipsec/xform_ipcomp.c b/sys/netipsec/xform_ipcomp.c index eb68f2aa444e..a18c340ddf58 100644 --- a/sys/netipsec/xform_ipcomp.c +++ b/sys/netipsec/xform_ipcomp.c @@ -194,34 +194,35 @@ ipcomp_input(struct mbuf *m, struct secasvar *sav, int skip, int protoff) 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 skip, int protoff) 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 skip, int protoff) SECASVAR_UNLOCK(sav); return crypto_dispatch(crp); +bad: + m_freem(m); + key_freesav(&sav); + return (error); } /*