From d35ebfbce1985758fd109a734f1cbd51b06e22e1 Mon Sep 17 00:00:00 2001 From: Adrian-Ken Rueegsegger Date: Fri, 1 Sep 2017 16:30:49 +0200 Subject: [PATCH 1/7] charon-tkm: Add missing whitespace log message --- src/charon-tkm/src/tkm/tkm_keymat.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/charon-tkm/src/tkm/tkm_keymat.c b/src/charon-tkm/src/tkm/tkm_keymat.c index a24760445..eb9f039d7 100644 --- a/src/charon-tkm/src/tkm/tkm_keymat.c +++ b/src/charon-tkm/src/tkm/tkm_keymat.c @@ -279,7 +279,7 @@ 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); this->ae_ctx_id = isa_info.ae_id; res = ike_isa_create_child(this->isa_ctx_id, isa_info.parent_isa_id, 1, From fcde9686f67f5724f7de47ed27737bc2457310dc Mon Sep 17 00:00:00 2001 From: Adrian-Ken Rueegsegger Date: Fri, 1 Sep 2017 16:58:15 +0200 Subject: [PATCH 2/7] charon-tkm: Store context ids as int instead of bool This is in preparation of making context ids refcountable. --- src/charon-tkm/src/tkm/tkm_id_manager.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 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..b07409d19 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; } @@ -117,7 +117,10 @@ METHOD(tkm_id_manager_t, release_id, bool, } this->locks[kind]->write_lock(this->locks[kind]); - this->ctxids[kind][idx] = false; + if (this->ctxids[kind][idx] > 0) + { + this->ctxids[kind][idx]--; + } this->locks[kind]->unlock(this->locks[kind]); return TRUE; @@ -155,7 +158,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]); From 1b2a8d963a1503456fb5af47cae82536076a2880 Mon Sep 17 00:00:00 2001 From: Adrian-Ken Rueegsegger Date: Fri, 1 Sep 2017 17:15:41 +0200 Subject: [PATCH 3/7] charon-tkm: Add acquire_ref method to ID manager The function acquires a reference to the given context reference id for a specific context kind. --- src/charon-tkm/src/tkm/tkm_id_manager.c | 28 ++++++++++ src/charon-tkm/src/tkm/tkm_id_manager.h | 12 +++++ src/charon-tkm/tests/id_manager_tests.c | 68 +++++++++++++++++++++++++ 3 files changed, 108 insertions(+) diff --git a/src/charon-tkm/src/tkm/tkm_id_manager.c b/src/charon-tkm/src/tkm/tkm_id_manager.c index b07409d19..2d28c3c45 100644 --- a/src/charon-tkm/src/tkm/tkm_id_manager.c +++ b/src/charon-tkm/src/tkm/tkm_id_manager.c @@ -103,6 +103,33 @@ METHOD(tkm_id_manager_t, acquire_id, int, return id; } +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, bool, private_tkm_id_manager_t * const this, const tkm_context_kind_t kind, const int id) @@ -150,6 +177,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, }, diff --git a/src/charon-tkm/src/tkm/tkm_id_manager.h b/src/charon-tkm/src/tkm/tkm_id_manager.h index 0fc9ff8ef..ec7b07f7e 100644 --- a/src/charon-tkm/src/tkm/tkm_id_manager.h +++ b/src/charon-tkm/src/tkm/tkm_id_manager.h @@ -73,6 +73,18 @@ struct tkm_id_manager_t { int (*acquire_id)(tkm_id_manager_t * const this, 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. * diff --git a/src/charon-tkm/tests/id_manager_tests.c b/src/charon-tkm/tests/id_manager_tests.c index 8157496ca..392379aa9 100644 --- a/src/charon-tkm/tests/id_manager_tests.c +++ b/src/charon-tkm/tests/id_manager_tests.c @@ -84,6 +84,71 @@ START_TEST(test_acquire_id_same) } END_TEST +START_TEST(test_acquire_ref) +{ + int i, id = 0; + 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, id = 0; @@ -150,6 +215,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"); From c198ddcb3fa7eb855d3e56d4ddf72aa7a5dfe554 Mon Sep 17 00:00:00 2001 From: Adrian-Ken Rueegsegger Date: Fri, 1 Sep 2017 17:21:05 +0200 Subject: [PATCH 4/7] charon-tkm: Return current refcount when releasing ID --- src/charon-tkm/src/tkm/tkm_id_manager.c | 9 ++++---- src/charon-tkm/src/tkm/tkm_id_manager.h | 8 +++---- src/charon-tkm/tests/id_manager_tests.c | 29 ++++++++++++------------- 3 files changed, 23 insertions(+), 23 deletions(-) diff --git a/src/charon-tkm/src/tkm/tkm_id_manager.c b/src/charon-tkm/src/tkm/tkm_id_manager.c index 2d28c3c45..9a2ede03e 100644 --- a/src/charon-tkm/src/tkm/tkm_id_manager.c +++ b/src/charon-tkm/src/tkm/tkm_id_manager.c @@ -130,27 +130,28 @@ METHOD(tkm_id_manager_t, acquire_ref, bool, return TRUE; } -METHOD(tkm_id_manager_t, release_id, bool, +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]); if (this->ctxids[kind][idx] > 0) { - this->ctxids[kind][idx]--; + refcount = --this->ctxids[kind][idx]; } this->locks[kind]->unlock(this->locks[kind]); - return TRUE; + return refcount; } diff --git a/src/charon-tkm/src/tkm/tkm_id_manager.h b/src/charon-tkm/src/tkm/tkm_id_manager.h index ec7b07f7e..1c48b57f1 100644 --- a/src/charon-tkm/src/tkm/tkm_id_manager.h +++ b/src/charon-tkm/src/tkm/tkm_id_manager.h @@ -90,11 +90,11 @@ struct tkm_id_manager_t { * * @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/tests/id_manager_tests.c b/src/charon-tkm/tests/id_manager_tests.c index 392379aa9..fb5e56a05 100644 --- a/src/charon-tkm/tests/id_manager_tests.c +++ b/src/charon-tkm/tests/id_manager_tests.c @@ -151,19 +151,18 @@ END_TEST START_TEST(test_release_id) { - int i, id = 0; - bool released = false; + 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); @@ -172,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); } @@ -190,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); } From 8e823bb8b1ae4b1efcb81bc2cf07717333309120 Mon Sep 17 00:00:00 2001 From: Adrian-Ken Rueegsegger Date: Fri, 1 Sep 2017 18:46:05 +0200 Subject: [PATCH 5/7] charon-tkm: Fix AE context life-cycle handling Use new reference counting feature of ID manager for AE contexts and only perform reset if count is zero. Also, do not pass on AE ID as every IKE SA must decrement AE ID count once it is not used any longer. --- src/charon-tkm/src/tkm/tkm_keymat.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/charon-tkm/src/tkm/tkm_keymat.c b/src/charon-tkm/src/tkm/tkm_keymat.c index eb9f039d7..ed5366c2c 100644 --- a/src/charon-tkm/src/tkm/tkm_keymat.c +++ b/src/charon-tkm/src/tkm/tkm_keymat.c @@ -281,6 +281,13 @@ METHOD(keymat_v2_t, derive_ike_keys, bool, 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, 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); From 59e7298ff9d879493391f06b95189c23dedb7740 Mon Sep 17 00:00:00 2001 From: Adrian-Ken Rueegsegger Date: Mon, 4 Sep 2017 15:00:42 +0200 Subject: [PATCH 6/7] charon-tkm: Check for error when acquiring ESA ID --- src/charon-tkm/src/tkm/tkm_kernel_ipsec.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/charon-tkm/src/tkm/tkm_kernel_ipsec.c b/src/charon-tkm/src/tkm/tkm_kernel_ipsec.c index 5decde92b..87b516d43 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)) { @@ -196,6 +202,7 @@ failure: 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; From fc08e6af8a92816b5f1ae2805c22cc82c3a7be4f Mon Sep 17 00:00:00 2001 From: Adrian-Ken Rueegsegger Date: Tue, 5 Sep 2017 15:56:12 +0200 Subject: [PATCH 7/7] charon-tkm: Reset ESA on child SA create failure Since we are also releasing the ESA ID we have to make sure that the ESA context is reset and in a clean state in order for it to be actually reusable. --- src/charon-tkm/src/tkm/tkm_kernel_ipsec.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/charon-tkm/src/tkm/tkm_kernel_ipsec.c b/src/charon-tkm/src/tkm/tkm_kernel_ipsec.c index 87b516d43..48dd40aa5 100644 --- a/src/charon-tkm/src/tkm/tkm_kernel_ipsec.c +++ b/src/charon-tkm/src/tkm/tkm_kernel_ipsec.c @@ -199,6 +199,7 @@ 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);