Fix possible use after free due to security policy deletion.

When we are passing mbuf to IPSec processing via ipsec[46]_process_packet(),
we hold one reference to security policy and release it just after return
from this function. But IPSec processing can be deffered and when we release
reference to security policy after ipsec[46]_process_packet(), user can
delete this security policy from SPDB. And when IPSec processing will be
done, xform's callback function will do access to already freed memory.

To fix this move KEY_FREESP() into callback function. Now IPSec code will
release reference to SP after processing will be finished.

Differential Revision:	https://reviews.freebsd.org/D2324
No objections from:	#network
Sponsored by:	Yandex LLC

4 files changed, 30 insertions, 8 deletions

diff --git a/sys/netipsec/ipsec_output.c b/sys/netipsec/ipsec_output.c
index 583f4b0..22a50c4 100644
--- a/sys/netipsec/ipsec_output.c
+++ b/sys/netipsec/ipsec_output.c
@@ -104,6 +104,7 @@ ipsec_process_done(struct mbuf *m, struct ipsecrequest *isr)
 
 	IPSEC_ASSERT(m != NULL, ("null mbuf"));
 	IPSEC_ASSERT(isr != NULL, ("null ISR"));
+	IPSEC_ASSERT(isr->sp != NULL, ("NULL isr->sp"));
 	sav = isr->sav;
 	IPSEC_ASSERT(sav != NULL, ("null SA"));
 	IPSEC_ASSERT(sav->sah != NULL, ("null SAH"));
@@ -163,6 +164,10 @@ ipsec_process_done(struct mbuf *m, struct ipsecrequest *isr)
 	 * If this is a problem we'll need to introduce a queue
 	 * to set the packet on so we can unwind the stack before
 	 * doing further processing.
+	 *
+	 * If ipsec[46]_process_packet() will successfully queue
+	 * the request, we need to take additional reference to SP,
+	 * because xform callback will release reference.
 	 */
 	if (isr->next) {
 		/* XXX-BZ currently only support same AF bundles. */
@@ -170,7 +175,11 @@ ipsec_process_done(struct mbuf *m, struct ipsecrequest *isr)
 #ifdef INET
 		case AF_INET:
 			IPSECSTAT_INC(ips_out_bundlesa);
-			return ipsec4_process_packet(m, isr->next);
+			key_addref(isr->sp);
+			error = ipsec4_process_packet(m, isr->next);
+			if (error != 0)
+				KEY_FREESP(&isr->sp);
+			return (error);
 			/* NOTREACHED */
 #endif
 #ifdef notyet
@@ -178,7 +187,11 @@ ipsec_process_done(struct mbuf *m, struct ipsecrequest *isr)
 		case AF_INET6:
 			/* XXX */
 			IPSEC6STAT_INC(ips_out_bundlesa);
-			return ipsec6_process_packet(m, isr->next);
+			key_addref(isr->sp);
+			error = ipsec6_process_packet(m, isr->next);
+			if (error != 0)
+				KEY_FREESP(&isr->sp);
+			return (error);
 			/* NOTREACHED */
 #endif /* INET6 */
 #endif
diff --git a/sys/netipsec/xform_ah.c b/sys/netipsec/xform_ah.c
index cb257a5..8f791db 100644
--- a/sys/netipsec/xform_ah.c
+++ b/sys/netipsec/xform_ah.c
@@ -1091,6 +1091,7 @@ ah_output_cb(struct cryptop *crp)
 	m = (struct mbuf *) crp->crp_buf;
 
 	isr = tc->tc_isr;
+	IPSEC_ASSERT(isr->sp != NULL, ("NULL isr->sp"));
 	IPSECREQUEST_LOCK(isr);
 	sav = tc->tc_sav;
 	/* With the isr lock released SA pointer can be updated. */
@@ -1154,16 +1155,18 @@ ah_output_cb(struct cryptop *crp)
 	error = ipsec_process_done(m, isr);
 	KEY_FREESAV(&sav);
 	IPSECREQUEST_UNLOCK(isr);
-	return error;
+	KEY_FREESP(&isr->sp);
+	return (error);
 bad:
 	if (sav)
 		KEY_FREESAV(&sav);
 	IPSECREQUEST_UNLOCK(isr);
+	KEY_FREESP(&isr->sp);
 	if (m)
 		m_freem(m);
 	free(tc, M_XDATA);
 	crypto_freereq(crp);
-	return error;
+	return (error);
 }
 
 static struct xformsw ah_xformsw = {
diff --git a/sys/netipsec/xform_esp.c b/sys/netipsec/xform_esp.c
index 2fbdb81..003c514 100644
--- a/sys/netipsec/xform_esp.c
+++ b/sys/netipsec/xform_esp.c
@@ -888,6 +888,7 @@ esp_output_cb(struct cryptop *crp)
 	m = (struct mbuf *) crp->crp_buf;
 
 	isr = tc->tc_isr;
+	IPSEC_ASSERT(isr->sp != NULL, ("NULL isr->sp"));
 	IPSECREQUEST_LOCK(isr);
 	sav = tc->tc_sav;
 	/* With the isr lock released SA pointer can be updated. */
@@ -966,16 +967,18 @@ esp_output_cb(struct cryptop *crp)
 	error = ipsec_process_done(m, isr);
 	KEY_FREESAV(&sav);
 	IPSECREQUEST_UNLOCK(isr);
-	return error;
+	KEY_FREESP(&isr->sp);
+	return (error);
 bad:
 	if (sav)
 		KEY_FREESAV(&sav);
 	IPSECREQUEST_UNLOCK(isr);
+	KEY_FREESP(&isr->sp);
 	if (m)
 		m_freem(m);
 	free(tc, M_XDATA);
 	crypto_freereq(crp);
-	return error;
+	return (error);
 }
 
 static struct xformsw esp_xformsw = {
diff --git a/sys/netipsec/xform_ipcomp.c b/sys/netipsec/xform_ipcomp.c
index 5f3afd9..ef460cd 100644
--- a/sys/netipsec/xform_ipcomp.c
+++ b/sys/netipsec/xform_ipcomp.c
@@ -492,6 +492,7 @@ ipcomp_output_cb(struct cryptop *crp)
 	skip = tc->tc_skip;
 
 	isr = tc->tc_isr;
+	IPSEC_ASSERT(isr->sp != NULL, ("NULL isr->sp"));
 	IPSECREQUEST_LOCK(isr);
 	sav = tc->tc_sav;
 	/* With the isr lock released SA pointer can be updated. */
@@ -606,16 +607,18 @@ ipcomp_output_cb(struct cryptop *crp)
 	error = ipsec_process_done(m, isr);
 	KEY_FREESAV(&sav);
 	IPSECREQUEST_UNLOCK(isr);
-	return error;
+	KEY_FREESP(&isr->sp);
+	return (error);
 bad:
 	if (sav)
 		KEY_FREESAV(&sav);
 	IPSECREQUEST_UNLOCK(isr);
+	KEY_FREESP(&isr->sp);
 	if (m)
 		m_freem(m);
 	free(tc, M_XDATA);
 	crypto_freereq(crp);
-	return error;
+	return (error);
 }
 
 static struct xformsw ipcomp_xformsw = {