Merge branch 'tkm-cid-ref'

This fixes IKE_SA rekey collision handling and improves error handling
if CHILD_SA setup fails.

5 files changed, 157 insertions, 35 deletions

diff --git a/src/charon-tkm/src/tkm/tkm_id_manager.c b/src/charon-tkm/src/tkm/tkm_id_manager.c
index d8ff6753f..9a2ede03e 100644
--- a/src/charon-tkm/src/tkm/tkm_id_manager.c
+++ b/src/charon-tkm/src/tkm/tkm_id_manager.c
@@ -43,7 +43,7 @@ struct private_tkm_id_manager_t {
 	/**
 	 * Per-kind array of free context ids
 	 */
-	bool* ctxids[TKM_CTX_MAX];
+	int* ctxids[TKM_CTX_MAX];
 
 	/**
 	 * Per-kind context limits.
@@ -85,9 +85,9 @@ METHOD(tkm_id_manager_t, acquire_id, int,
 	this->locks[kind]->write_lock(this->locks[kind]);
 	for (j = 0; j < this->limits[kind]; j++)
 	{
-		if (!this->ctxids[kind][j])
+		if (this->ctxids[kind][j] == 0)
 		{
-			this->ctxids[kind][j] = true;
+			this->ctxids[kind][j] = 1;
 			id = j + 1;
 			break;
 		}
@@ -103,24 +103,55 @@ METHOD(tkm_id_manager_t, acquire_id, int,
 	return id;
 }
 
-METHOD(tkm_id_manager_t, release_id, bool,
+METHOD(tkm_id_manager_t, acquire_ref, bool,
+	private_tkm_id_manager_t * const this, const tkm_context_kind_t kind,
+	const int ref_id)
+{
+	const int idx = ref_id - 1;
+
+	if (!is_valid_kind(kind))
+	{
+		DBG1(DBG_LIB, "tried to acquire reference for invalid context kind '%d'",
+			 kind);
+		return FALSE;
+	}
+
+	if (ref_id < 1 || (uint64_t)ref_id > this->limits[kind])
+	{
+		DBG1(DBG_LIB, "tried to acquire reference for context id %d out of "
+			 "bounds (max %llu)", ref_id, this->limits[kind]);
+		return FALSE;
+	}
+
+	this->locks[kind]->write_lock(this->locks[kind]);
+	this->ctxids[kind][idx]++;
+	this->locks[kind]->unlock(this->locks[kind]);
+
+	return TRUE;
+}
+
+METHOD(tkm_id_manager_t, release_id, int,
 	private_tkm_id_manager_t * const this, const tkm_context_kind_t kind,
 	const int id)
 {
 	const int idx = id - 1;
+	int refcount = 0;
 
 	if (!is_valid_kind(kind))
 	{
 		DBG1(DBG_LIB, "tried to release id %d for invalid context kind '%d'",
 			 id, kind);
-		return FALSE;
+		return -1;
 	}
 
 	this->locks[kind]->write_lock(this->locks[kind]);
-	this->ctxids[kind][idx] = false;
+	if (this->ctxids[kind][idx] > 0)
+	{
+		refcount = --this->ctxids[kind][idx];
+	}
 	this->locks[kind]->unlock(this->locks[kind]);
 
-	return TRUE;
+	return refcount;
 }
 
 
@@ -147,6 +178,7 @@ tkm_id_manager_t *tkm_id_manager_create(const tkm_limits_t limits)
 	INIT(this,
 		.public = {
 			.acquire_id = _acquire_id,
+			.acquire_ref = _acquire_ref,
 			.release_id = _release_id,
 			.destroy = _destroy,
 		},
@@ -155,7 +187,7 @@ tkm_id_manager_t *tkm_id_manager_create(const tkm_limits_t limits)
 	for (i = 0; i < TKM_CTX_MAX; i++)
 	{
 		this->limits[i] = limits[i];
-		this->ctxids[i] = calloc(limits[i], sizeof(bool));
+		this->ctxids[i] = calloc(limits[i], sizeof(int));
 		this->locks[i] = rwlock_create(RWLOCK_TYPE_DEFAULT);
 		DBG2(DBG_LIB, "%N initialized, %llu slot(s)", tkm_context_kind_names, i,
 			 limits[i]);
diff --git a/src/charon-tkm/src/tkm/tkm_id_manager.h b/src/charon-tkm/src/tkm/tkm_id_manager.h
index 0fc9ff8ef..1c48b57f1 100644
--- a/src/charon-tkm/src/tkm/tkm_id_manager.h
+++ b/src/charon-tkm/src/tkm/tkm_id_manager.h
@@ -74,15 +74,27 @@ struct tkm_id_manager_t {
 					  const tkm_context_kind_t kind);
 
 	/**
+	 * Acquire reference to given context id for a specific context kind.
+	 *
+	 * @param kind			kind of context id
+	 * @param ref_id		id to acquire a reference for
+	 * @return				TRUE if reference could be acquired,
+	 *						FALSE otherwise
+	 */
+	bool (*acquire_ref)(tkm_id_manager_t * const this,
+						const tkm_context_kind_t kind,
+						const int ref_id);
+
+	/**
 	 * Release a previously acquired context id.
 	 *
 	 * @param kind			kind of context id to release
 	 * @param id			id to release
-	 * @return				TRUE if id was released, FALSE otherwise
+	 * @return				current refcount if id was released, -1 otherwise
 	 */
-	bool (*release_id)(tkm_id_manager_t * const this,
-					   const tkm_context_kind_t kind,
-					   const int id);
+	int (*release_id)(tkm_id_manager_t * const this,
+					  const tkm_context_kind_t kind,
+					  const int id);
 
 	/**
 	 * Destroy a tkm_id_manager instance.
diff --git a/src/charon-tkm/src/tkm/tkm_kernel_ipsec.c b/src/charon-tkm/src/tkm/tkm_kernel_ipsec.c
index 5decde92b..48dd40aa5 100644
--- a/src/charon-tkm/src/tkm/tkm_kernel_ipsec.c
+++ b/src/charon-tkm/src/tkm/tkm_kernel_ipsec.c
@@ -134,6 +134,12 @@ METHOD(kernel_ipsec_t, add_sa, status_t,
 	}
 
 	esa_id = tkm->idmgr->acquire_id(tkm->idmgr, TKM_CTX_ESA);
+	if (esa_id == 0)
+	{
+		DBG1(DBG_KNL, "unable to acquire esa context id");
+		goto esa_id_failure;
+	}
+
 	if (!tkm->sad->insert(tkm->sad, esa_id, data->reqid, local, peer,
 						  spi_loc, spi_rem, id->proto))
 	{
@@ -193,9 +199,11 @@ METHOD(kernel_ipsec_t, add_sa, status_t,
 	return SUCCESS;
 
 failure:
+	ike_esa_reset(esa_id);
 	tkm->sad->remove(tkm->sad, esa_id);
 sad_failure:
 	tkm->idmgr->release_id(tkm->idmgr, TKM_CTX_ESA, esa_id);
+esa_id_failure:
 	chunk_free(&esa.nonce_i);
 	chunk_free(&esa.nonce_r);
 	return FAILED;
diff --git a/src/charon-tkm/src/tkm/tkm_keymat.c b/src/charon-tkm/src/tkm/tkm_keymat.c
index a24760445..ed5366c2c 100644
--- a/src/charon-tkm/src/tkm/tkm_keymat.c
+++ b/src/charon-tkm/src/tkm/tkm_keymat.c
@@ -279,8 +279,15 @@ METHOD(keymat_v2_t, derive_ike_keys, bool,
 		}
 		isa_info = *((isa_info_t *)(rekey_skd.ptr));
 		DBG1(DBG_IKE, "deriving IKE keys (parent_isa: %llu, ae: %llu, nc: %llu,"
-			 "dh: %llu, spi_loc: %llx, spi_rem: %llx)", isa_info.parent_isa_id,
+			 " dh: %llu, spi_loc: %llx, spi_rem: %llx)", isa_info.parent_isa_id,
 			 isa_info.ae_id, nc_id, dh_id, spi_loc, spi_rem);
+
+		if (!tkm->idmgr->acquire_ref(tkm->idmgr, TKM_CTX_AE, isa_info.ae_id))
+		{
+			DBG1(DBG_IKE, "unable to acquire reference for ae: %llu",
+				 isa_info.ae_id);
+			return FALSE;
+		}
 		this->ae_ctx_id = isa_info.ae_id;
 		res = ike_isa_create_child(this->isa_ctx_id, isa_info.parent_isa_id, 1,
 								   dh_id, nc_id, nonce_rem, this->initiator,
@@ -416,11 +423,6 @@ METHOD(keymat_v2_t, get_skd, pseudo_random_function_t,
 
 	*skd = chunk_create((u_char *)isa_info, sizeof(isa_info_t));
 
-	/*
-	 * remove ae context id, since control has now been handed over to the new
-	 * IKE SA keymat
-	 */
-	this->ae_ctx_id = 0;
 	return PRF_HMAC_SHA2_512;
 }
 
@@ -462,11 +464,12 @@ METHOD(keymat_t, destroy, void,
 	/* only reset ae context if set */
 	if (this->ae_ctx_id != 0)
 	{
-		if (ike_ae_reset(this->ae_ctx_id) != TKM_OK)
+		int count;
+		count = tkm->idmgr->release_id(tkm->idmgr, TKM_CTX_AE, this->ae_ctx_id);
+		if (count == 0 && ike_ae_reset(this->ae_ctx_id) != TKM_OK)
 		{
 			DBG1(DBG_IKE, "failed to reset AE context %d", this->ae_ctx_id);
 		}
-		tkm->idmgr->release_id(tkm->idmgr, TKM_CTX_AE, this->ae_ctx_id);
 	}
 
 	DESTROY_IF(this->hash_algorithms);
diff --git a/src/charon-tkm/tests/id_manager_tests.c b/src/charon-tkm/tests/id_manager_tests.c
index 8157496ca..fb5e56a05 100644
--- a/src/charon-tkm/tests/id_manager_tests.c
+++ b/src/charon-tkm/tests/id_manager_tests.c
@@ -84,21 +84,85 @@ START_TEST(test_acquire_id_same)
 }
 END_TEST
 
-START_TEST(test_release_id)
+START_TEST(test_acquire_ref)
 {
 	int i, id = 0;
-	bool released = false;
+	bool acquired = false;
+	tkm_id_manager_t *idmgr = tkm_id_manager_create(limits);
+
+	for (i = 0; i < TKM_CTX_MAX; i++)
+	{
+		id = idmgr->acquire_id(idmgr, i);
+		acquired = idmgr->acquire_ref(idmgr, i, id);
+		fail_unless(acquired, "Error acquiring reference context kind %d", i);
+
+		/* Reset test variable */
+		acquired = false;
+	}
+
+	idmgr->destroy(idmgr);
+}
+END_TEST
+
+START_TEST(test_acquire_ref_invalid_kind)
+{
+	bool acquired;
+	tkm_id_manager_t *idmgr = tkm_id_manager_create(limits);
+
+	acquired = idmgr->acquire_ref(idmgr, TKM_CTX_MAX, 1);
+	fail_if(acquired, "Acquired reference for invalid context kind %d", TKM_CTX_MAX);
+
+	/* Reset test variable */
+	acquired = 0;
+
+	acquired = idmgr->acquire_ref(idmgr, -1, 1);
+	fail_if(acquired, "Acquired reference for invalid context kind %d", -1);
+
+	idmgr->destroy(idmgr);
+}
+END_TEST
+
+START_TEST(test_acquire_ref_invalid_id)
+{
+	int i;
+	bool acquired;
+	tkm_id_manager_t *idmgr = tkm_id_manager_create(limits);
+
+	for (i = 0; i < TKM_CTX_MAX; i++)
+	{
+		acquired = idmgr->acquire_ref(idmgr, i, -1);
+		fail_if(acquired,
+				"Acquired reference for negative id of context kind %d", i);
+
+		/* Reset test variable */
+		acquired = false;
+
+		acquired = idmgr->acquire_ref(idmgr, i, limits[i] + 1);
+		fail_if(acquired,
+				"Acquired reference exceeding limit of context kind %d", i);
+
+		/* Reset test variable */
+		acquired = false;
+	}
+
+	idmgr->destroy(idmgr);
+}
+END_TEST
+
+START_TEST(test_release_id)
+{
+	int i, count, id = 0;
 	tkm_id_manager_t *idmgr = tkm_id_manager_create(limits);
 
 	for (i = 0; i < TKM_CTX_MAX; i++)
 	{
 		id = idmgr->acquire_id(idmgr, i);
-		released = idmgr->release_id(idmgr, i, id);
+		count = idmgr->release_id(idmgr, i, id);
 
-		fail_unless(released, "Error releasing id of context kind %d", i);
+		fail_unless(count == 0, "Error releasing id of context kind %d", i);
 
-		/* Reset released variable */
-		released = FALSE;
+		/* Reset count variable */
+		count = 0;
 	}
 
 	idmgr->destroy(idmgr);
@@ -107,17 +171,17 @@ END_TEST
 
 START_TEST(test_release_id_invalid_kind)
 {
-	bool released = TRUE;
+	int count = 0;
 	tkm_id_manager_t *idmgr = tkm_id_manager_create(limits);
 
-	released = idmgr->release_id(idmgr, TKM_CTX_MAX, 1);
-	fail_if(released, "Released id for invalid context kind %d", TKM_CTX_MAX);
+	count = idmgr->release_id(idmgr, TKM_CTX_MAX, 1);
+	fail_if(count >= 0, "Released id for invalid context kind %d", TKM_CTX_MAX);
 
 	/* Reset test variable */
-	released = TRUE;
+	count = 0;
 
-	released = idmgr->release_id(idmgr, -1, 1);
-	fail_if(released, "Released id for invalid context kind %d", -1);
+	count = idmgr->release_id(idmgr, -1, 1);
+	fail_if(count >= 0, "Released id for invalid context kind %d", -1);
 
 	idmgr->destroy(idmgr);
 }
@@ -125,11 +189,11 @@ END_TEST
 
 START_TEST(test_release_id_nonexistent)
 {
-	bool released = FALSE;
+	int count = 0;
 	tkm_id_manager_t *idmgr = tkm_id_manager_create(limits);
 
-	released = idmgr->release_id(idmgr, TKM_CTX_NONCE, 1);
-	fail_unless(released, "Release of nonexistent id failed");
+	count = idmgr->release_id(idmgr, TKM_CTX_NONCE, 1);
+	fail_unless(count == 0, "Release of nonexistent id failed");
 
 	idmgr->destroy(idmgr);
 }
@@ -150,6 +214,9 @@ Suite *make_id_manager_tests()
 	tcase_add_test(tc, test_acquire_id);
 	tcase_add_test(tc, test_acquire_id_invalid_kind);
 	tcase_add_test(tc, test_acquire_id_same);
+	tcase_add_test(tc, test_acquire_ref);
+	tcase_add_test(tc, test_acquire_ref_invalid_kind);
+	tcase_add_test(tc, test_acquire_ref_invalid_id);
 	suite_add_tcase(s, tc);
 
 	tc = tcase_create("release");