IPsec crash, patch for review

[VANHULLEBUS Yvan]

Hi all.

We (NETASQ) had some IPsec related kernel crashes, and hunted them,
here are some informations and a possible patch:


First, problem only occurs when asynchronous crypto is done
(hardware encryption such as hifn cards, or software patch to do
encryption on a separate kthread when having multiple CPUs).

More or less exploitable backtraces always shows a problem with an
isr which seems to be used but already freed.

While looking at the code, we noticed that there is probably a race
condition when using asynchronous crypto: ip_output (well,
ip_ipsec_output() now) releases refcnt to the SPD entry when IPsec
processing needs to be done, but SPD's isr will still be used by
[esp|ah|ipcomp]_output_cb().

The attached patch moves the KEY_FREESP() from ip_ipsec_output() to
[esp|ah|ipcomp]_output_cb() when there will be IPsec processing (when
ip_ipsec_output()returns -1).
(note for George and Bjoern: this is a newer version of the patch,
which also fixes a refcnt leak).


This patches solves our problem (no more crashes for a week, when we
had at least 2-3 crashes / day), and has also successfully passed our
non regression testsuite.

As it may still have some unexpected impacts on other parts of the
code, I'd like to have some feedback on it before commiting it.


Thanks,

Yvan.
-------------- next part --------------
Index: netinet/ip_ipsec.c
===================================================================
--- netinet/ip_ipsec.c	(revision 193698)
+++ netinet/ip_ipsec.c	(working copy)
@@ -405,8 +405,6 @@ done:
 		KEY_FREESP(&sp);
 	return 0;
 reinjected:
-	if (sp != NULL)
-		KEY_FREESP(&sp);
 	return -1;
 bad:
 	if (sp != NULL)
Index: netipsec/xform_esp.c
===================================================================
--- netipsec/xform_esp.c	(revision 193698)
+++ netipsec/xform_esp.c	(working copy)
@@ -979,11 +979,15 @@ esp_output_cb(struct cryptop *crp)
 	err = ipsec_process_done(m, isr);
 	KEY_FREESAV(&sav);
 	IPSECREQUEST_UNLOCK(isr);
+	if(isr->sp != NULL) /* should NEVER be NULL !!! */
+		KEY_FREESP(isr->sp);
 	return err;
 bad:
 	if (sav)
 		KEY_FREESAV(&sav);
 	IPSECREQUEST_UNLOCK(isr);
+	if(isr->sp != NULL) /* should NEVER be NULL !!! */
+		KEY_FREESP(isr->sp);
 	if (m)
 		m_freem(m);
 	free(tc, M_XDATA);
Index: netipsec/xform_ipcomp.c
===================================================================
--- netipsec/xform_ipcomp.c	(revision 193698)
+++ netipsec/xform_ipcomp.c	(working copy)
@@ -588,11 +588,15 @@ ipcomp_output_cb(struct cryptop *crp)
 	error = ipsec_process_done(m, isr);
 	KEY_FREESAV(&sav);
 	IPSECREQUEST_UNLOCK(isr);
+	if(isr->sp != NULL) /* should NEVER be NULL !!! */
+		KEY_FREESP(isr->sp);
 	return error;
 bad:
 	if (sav)
 		KEY_FREESAV(&sav);
 	IPSECREQUEST_UNLOCK(isr);
+	if(isr->sp != NULL) /* should NEVER be NULL !!! */
+		KEY_FREESP(isr->sp);
 	if (m)
 		m_freem(m);
 	free(tc, M_XDATA);
Index: netipsec/xform_ah.c
===================================================================
--- netipsec/xform_ah.c	(revision 193698)
+++ netipsec/xform_ah.c	(working copy)
@@ -1210,11 +1210,15 @@ ah_output_cb(struct cryptop *crp)
 	err = ipsec_process_done(m, isr);
 	KEY_FREESAV(&sav);
 	IPSECREQUEST_UNLOCK(isr);
+	if(isr->sp != NULL) /* should NEVER be NULL !!! */
+		KEY_FREESP(isr->sp);
 	return err;
 bad:
 	if (sav)
 		KEY_FREESAV(&sav);
 	IPSECREQUEST_UNLOCK(isr);
+	if(isr->sp != NULL) /* should NEVER be NULL !!! */
+		KEY_FREESP(isr->sp);
 	if (m)
 		m_freem(m);
 	free(tc, M_XDATA);
-------------- next part --------------
Index: netinet/ip_ipsec.c
===================================================================
--- netinet/ip_ipsec.c	(revision 194337)
+++ netinet/ip_ipsec.c	(working copy)
@@ -347,6 +347,11 @@ ip_ipsec_output(struct mbuf **m, struct 
 
 		/* NB: callee frees mbuf */
 		*error = ipsec4_process_packet(*m, sp->req, *flags, 0);
+
+		/* Release SP now if an error occured (including acquire) */
+		if(error)
+			KEY_FREESP(&sp);
+
 		if (*error == EJUSTRETURN) {
 			/*
 			 * We had a SP with a level of 'use' and no SA. We
@@ -358,6 +363,7 @@ ip_ipsec_output(struct mbuf **m, struct 
 			ip->ip_off = ntohs(ip->ip_off);
 			goto done;
 		}
+
 		/*
 		 * Preserve KAME behaviour: ENOENT can be returned
 		 * when an SA acquire is in progress.  Don't propagate
@@ -405,8 +411,6 @@ done:
 		KEY_FREESP(&sp);
 	return 0;
 reinjected:
-	if (sp != NULL)
-		KEY_FREESP(&sp);
 	return -1;
 bad:
 	if (sp != NULL)
Index: netipsec/xform_esp.c
===================================================================
--- netipsec/xform_esp.c	(revision 194337)
+++ netipsec/xform_esp.c	(working copy)
@@ -979,11 +979,15 @@ esp_output_cb(struct cryptop *crp)
 	err = ipsec_process_done(m, isr);
 	KEY_FREESAV(&sav);
 	IPSECREQUEST_UNLOCK(isr);
+	IPSEC_ASSERT(isr->sp != NULL, ("NULL isr->sp"));
+	KEY_FREESP(&isr->sp);
 	return err;
 bad:
 	if (sav)
 		KEY_FREESAV(&sav);
 	IPSECREQUEST_UNLOCK(isr);
+	IPSEC_ASSERT(isr->sp != NULL, ("NULL isr->sp"));
+	KEY_FREESP(&isr->sp);
 	if (m)
 		m_freem(m);
 	free(tc, M_XDATA);
Index: netipsec/xform_ipcomp.c
===================================================================
--- netipsec/xform_ipcomp.c	(revision 194337)
+++ netipsec/xform_ipcomp.c	(working copy)
@@ -588,11 +588,15 @@ ipcomp_output_cb(struct cryptop *crp)
 	error = ipsec_process_done(m, isr);
 	KEY_FREESAV(&sav);
 	IPSECREQUEST_UNLOCK(isr);
+	IPSEC_ASSERT(isr->sp != NULL, ("NULL isr->sp"));
+	KEY_FREESP(&isr->sp);
 	return error;
 bad:
 	if (sav)
 		KEY_FREESAV(&sav);
 	IPSECREQUEST_UNLOCK(isr);
+	IPSEC_ASSERT(isr->sp != NULL, ("NULL isr->sp"));
+	KEY_FREESP(&isr->sp);
 	if (m)
 		m_freem(m);
 	free(tc, M_XDATA);
Index: netipsec/xform_ah.c
===================================================================
--- netipsec/xform_ah.c	(revision 194337)
+++ netipsec/xform_ah.c	(working copy)
@@ -1210,11 +1210,15 @@ ah_output_cb(struct cryptop *crp)
 	err = ipsec_process_done(m, isr);
 	KEY_FREESAV(&sav);
 	IPSECREQUEST_UNLOCK(isr);
+	IPSEC_ASSERT(isr->sp != NULL, ("NULL isr->sp"));
+	KEY_FREESP(&isr->sp);
 	return err;
 bad:
 	if (sav)
 		KEY_FREESAV(&sav);
 	IPSECREQUEST_UNLOCK(isr);
+	IPSEC_ASSERT(isr->sp != NULL, ("NULL isr->sp"));
+	KEY_FREESP(&isr->sp);
 	if (m)
 		m_freem(m);
 	free(tc, M_XDATA);