Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 23 May 2017 09:01:48 +0000 (UTC)
From:      "Andrey V. Elsukov" <ae@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r318734 - head/sys/netipsec
Message-ID:  <201705230901.v4N91mE0016182@repo.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: ae
Date: Tue May 23 09:01:48 2017
New Revision: 318734
URL: https://svnweb.freebsd.org/changeset/base/318734

Log:
  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

Modified:
  head/sys/netipsec/ipsec_input.c
  head/sys/netipsec/xform_ah.c
  head/sys/netipsec/xform_esp.c
  head/sys/netipsec/xform_ipcomp.c

Modified: head/sys/netipsec/ipsec_input.c
==============================================================================
--- head/sys/netipsec/ipsec_input.c	Tue May 23 08:11:55 2017	(r318733)
+++ head/sys/netipsec/ipsec_input.c	Tue May 23 09:01:48 2017	(r318734)
@@ -223,8 +223,6 @@ ipsec_common_input(struct mbuf *m, int s
 	 * everything else.
 	 */
 	error = (*sav->tdb_xform->xf_input)(m, sav, skip, protoff);
-	if (error != 0)
-		key_freesav(&sav);
 	return (error);
 }
 

Modified: head/sys/netipsec/xform_ah.c
==============================================================================
--- head/sys/netipsec/xform_ah.c	Tue May 23 08:11:55 2017	(r318733)
+++ head/sys/netipsec/xform_ah.c	Tue May 23 09:01:48 2017	(r318734)
@@ -566,8 +566,8 @@ ah_input(struct mbuf *m, struct secasvar
 	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
 		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
 		    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
 		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
 		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
 		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
 	xd->skip = skip;
 	xd->cryptoid = cryptoid;
 	return (crypto_dispatch(crp));
+bad:
+	m_freem(m);
+	key_freesav(&sav);
+	return (error);
 }
 
 /*

Modified: head/sys/netipsec/xform_esp.c
==============================================================================
--- head/sys/netipsec/xform_esp.c	Tue May 23 08:11:55 2017	(r318733)
+++ head/sys/netipsec/xform_esp.c	Tue May 23 09:01:48 2017	(r318734)
@@ -272,18 +272,18 @@ esp_input(struct mbuf *m, struct secasva
 	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 secasva
 		    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 secasva
 			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 secasva
 		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 secasva
 		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 secasva
 	crde->crd_alg = espx->type;
 
 	return (crypto_dispatch(crp));
+bad:
+	m_freem(m);
+	key_freesav(&sav);
+	return (error);
 }
 
 /*

Modified: head/sys/netipsec/xform_ipcomp.c
==============================================================================
--- head/sys/netipsec/xform_ipcomp.c	Tue May 23 08:11:55 2017	(r318733)
+++ head/sys/netipsec/xform_ipcomp.c	Tue May 23 09:01:48 2017	(r318734)
@@ -194,34 +194,35 @@ ipcomp_input(struct mbuf *m, struct seca
 	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 seca
 		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 seca
 	SECASVAR_UNLOCK(sav);
 
 	return crypto_dispatch(crp);
+bad:
+	m_freem(m);
+	key_freesav(&sav);
+	return (error);
 }
 
 /*



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201705230901.v4N91mE0016182>