From 90cf0078e1aea32c650ad0cba13619c0bc845d1b Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Mon, 2 Oct 2023 13:59:23 +0200 Subject: [PATCH 01/14] kernel-interface: Use reqid as sole key in hash table Every reqid is allocated once, we don't store the same reqid with e.g. different marks or interface IDs that would make it necessary to make them part of the key in that table (that's different in the other table). To preserve the current behavior, that is, allocating a new reqid if e.g. the marks are different, the additional selector values (which will result in an additional policy in the Linux kernel) are compared after the initial lookup. --- src/libcharon/kernel/kernel_interface.c | 32 +++++++++++-------------- 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/src/libcharon/kernel/kernel_interface.c b/src/libcharon/kernel/kernel_interface.c index 08570b4bc..d4a7286ef 100644 --- a/src/libcharon/kernel/kernel_interface.c +++ b/src/libcharon/kernel/kernel_interface.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2008-2019 Tobias Brunner + * Copyright (C) 2008-2023 Tobias Brunner * Copyright (C) 2010 Martin Willi * * Copyright (C) secunet Security Networks AG @@ -232,9 +232,9 @@ static void reqid_entry_destroy(reqid_entry_t *entry) } /** - * Hash the shared data of reqid entries + * Hash the additional selector properties of reqid entries */ -static u_int entry_hash_shared(reqid_entry_t *entry) +static u_int entry_hash_selectors(reqid_entry_t *entry) { u_int hash; @@ -250,9 +250,9 @@ static u_int entry_hash_shared(reqid_entry_t *entry) } /** - * Compare the shared properties of reqid entries + * Compare the additional selector properties of reqid entries */ -static bool entry_equals_shared(reqid_entry_t *a, reqid_entry_t *b) +static bool entry_equals_selectors(reqid_entry_t *a, reqid_entry_t *b) { return a->mark_in.value == b->mark_in.value && a->mark_in.mask == b->mark_in.mask && @@ -268,8 +268,7 @@ static bool entry_equals_shared(reqid_entry_t *a, reqid_entry_t *b) */ static u_int hash_reqid(reqid_entry_t *entry) { - return chunk_hash_inc(chunk_from_thing(entry->reqid), - entry_hash_shared(entry)); + return chunk_hash(chunk_from_thing(entry->reqid)); } /** @@ -277,11 +276,7 @@ static u_int hash_reqid(reqid_entry_t *entry) */ static bool equals_reqid(reqid_entry_t *a, reqid_entry_t *b) { - if (a->reqid == b->reqid) - { - return entry_equals_shared(a, b); - } - return FALSE; + return a->reqid == b->reqid; } /** @@ -309,7 +304,7 @@ static u_int hash_reqid_by_ts(reqid_entry_t *entry) { return hash_ts_array(entry->local, hash_ts_array(entry->remote, - entry_hash_shared(entry))); + entry_hash_selectors(entry))); } /** @@ -346,7 +341,7 @@ static bool equals_reqid_by_ts(reqid_entry_t *a, reqid_entry_t *b) if (ts_array_equals(a->local, b->local) && ts_array_equals(a->remote, b->remote)) { - return entry_equals_shared(a, b); + return entry_equals_selectors(a, b); } return FALSE; } @@ -397,16 +392,17 @@ METHOD(kernel_interface_t, alloc_reqid, status_t, /* search by reqid if given */ entry = this->reqids->get(this->reqids, tmpl); } - if (entry) + if (entry && entry_equals_selectors(entry, tmpl)) { - /* we don't require a traffic selector match for explicit reqids, + /* we don't require a traffic selector match for existing reqids, * as we want to reuse a reqid for trap-triggered policies that - * got narrowed during negotiation. */ + * got narrowed during negotiation, but we don't want to reuse the + * reqid if the additional selectors (e.g. marks) are different */ reqid_entry_destroy(tmpl); } else { - /* search by traffic selectors */ + /* search by traffic and other selectors */ entry = this->reqids_by_ts->get(this->reqids_by_ts, tmpl); if (entry) { From 02180ae2fff9d732e1e1be41466543b7e9bdd8ba Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Mon, 2 Oct 2023 14:08:56 +0200 Subject: [PATCH 02/14] kernel-interface: Remove unnecessary parameters for release_reqid() These are not included in the initial lookup anymore. Also simplified the implementation as we always add the same entry to the two hash tables. --- src/libcharon/kernel/kernel_interface.c | 39 ++++++------------------- src/libcharon/kernel/kernel_interface.h | 12 ++------ src/libcharon/sa/child_sa.c | 12 ++------ 3 files changed, 14 insertions(+), 49 deletions(-) diff --git a/src/libcharon/kernel/kernel_interface.c b/src/libcharon/kernel/kernel_interface.c index d4a7286ef..844af7060 100644 --- a/src/libcharon/kernel/kernel_interface.c +++ b/src/libcharon/kernel/kernel_interface.c @@ -434,45 +434,24 @@ METHOD(kernel_interface_t, alloc_reqid, status_t, } METHOD(kernel_interface_t, release_reqid, status_t, - private_kernel_interface_t *this, uint32_t reqid, - mark_t mark_in, mark_t mark_out, uint32_t if_id_in, uint32_t if_id_out, - sec_label_t *label) + private_kernel_interface_t *this, uint32_t reqid) { reqid_entry_t *entry, tmpl = { .reqid = reqid, - .mark_in = mark_in, - .mark_out = mark_out, - .if_id_in = if_id_in, - .if_id_out = if_id_out, - .label = label, }; this->mutex->lock(this->mutex); - entry = this->reqids->remove(this->reqids, &tmpl); - if (entry) + entry = this->reqids->get(this->reqids, &tmpl); + if (entry && --entry->refs == 0) { - if (--entry->refs == 0) - { - array_insert_create_value(&this->released_reqids, sizeof(uint32_t), - ARRAY_TAIL, &entry->reqid); - entry = this->reqids_by_ts->remove(this->reqids_by_ts, entry); - if (entry) - { - reqid_entry_destroy(entry); - } - } - else - { - this->reqids->put(this->reqids, entry, entry); - } + array_insert_create_value(&this->released_reqids, sizeof(uint32_t), + ARRAY_TAIL, &entry->reqid); + this->reqids->remove(this->reqids, entry); + this->reqids_by_ts->remove(this->reqids_by_ts, entry); + reqid_entry_destroy(entry); } this->mutex->unlock(this->mutex); - - if (entry) - { - return SUCCESS; - } - return NOT_FOUND; + return entry ? SUCCESS : NOT_FOUND; } METHOD(kernel_interface_t, add_sa, status_t, diff --git a/src/libcharon/kernel/kernel_interface.h b/src/libcharon/kernel/kernel_interface.h index 2bc9d8657..9f8e9b714 100644 --- a/src/libcharon/kernel/kernel_interface.h +++ b/src/libcharon/kernel/kernel_interface.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2006-2016 Tobias Brunner + * Copyright (C) 2006-2023 Tobias Brunner * Copyright (C) 2006 Daniel Roethlisberger * Copyright (C) 2005-2006 Martin Willi * Copyright (C) 2005 Jan Hutter @@ -162,17 +162,9 @@ struct kernel_interface_t { * Release a previously allocated reqid. * * @param reqid reqid to release - * @param mark_in inbound mark on SA - * @param mark_out outbound mark on SA - * @param if_id_in inbound interface ID on SA - * @param if_id_out outbound interface ID on SA - * @param label security label (usually the one on the policy, not SA) * @return SUCCESS if reqid released */ - status_t (*release_reqid)(kernel_interface_t *this, uint32_t reqid, - mark_t mark_in, mark_t mark_out, - uint32_t if_id_in, uint32_t if_id_out, - sec_label_t *label); + status_t (*release_reqid)(kernel_interface_t *this, uint32_t reqid); /** * Add an SA to the SAD. diff --git a/src/libcharon/sa/child_sa.c b/src/libcharon/sa/child_sa.c index d76d7aebc..e23accf0e 100644 --- a/src/libcharon/sa/child_sa.c +++ b/src/libcharon/sa/child_sa.c @@ -1810,9 +1810,7 @@ METHOD(child_sa_t, update, status_t, { if (new_reqid && charon->kernel->release_reqid(charon->kernel, - new_reqid, this->mark_in, this->mark_out, - this->if_id_in, this->if_id_out, - label_for(this, LABEL_USE_REQID)) != SUCCESS) + new_reqid) != SUCCESS) { DBG1(DBG_CHD, "releasing reqid %u failed", new_reqid); } @@ -1827,9 +1825,7 @@ METHOD(child_sa_t, update, status_t, if (new_reqid) { if (charon->kernel->release_reqid(charon->kernel, - this->reqid, this->mark_in, this->mark_out, - this->if_id_in, this->if_id_out, - label_for(this, LABEL_USE_REQID)) != SUCCESS) + this->reqid) != SUCCESS) { DBG1(DBG_CHD, "releasing reqid %u failed", this->reqid); } @@ -1950,9 +1946,7 @@ METHOD(child_sa_t, destroy, void, if (this->reqid_allocated) { if (charon->kernel->release_reqid(charon->kernel, - this->reqid, this->mark_in, this->mark_out, - this->if_id_in, this->if_id_out, - label_for(this, LABEL_USE_REQID)) != SUCCESS) + this->reqid) != SUCCESS) { DBG1(DBG_CHD, "releasing reqid %u failed", this->reqid); } From e623f5792bdacfc7358b21ea5d10b43d3e5a3050 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Mon, 2 Oct 2023 14:58:50 +0200 Subject: [PATCH 03/14] kernel-interface: Add method to increase refcount for allocated reqid --- src/libcharon/kernel/kernel_interface.c | 18 ++++++++++++++++++ src/libcharon/kernel/kernel_interface.h | 12 ++++++++++++ 2 files changed, 30 insertions(+) diff --git a/src/libcharon/kernel/kernel_interface.c b/src/libcharon/kernel/kernel_interface.c index 844af7060..ba5c03b9b 100644 --- a/src/libcharon/kernel/kernel_interface.c +++ b/src/libcharon/kernel/kernel_interface.c @@ -433,6 +433,23 @@ METHOD(kernel_interface_t, alloc_reqid, status_t, return SUCCESS; } +METHOD(kernel_interface_t, ref_reqid, status_t, + private_kernel_interface_t *this, uint32_t reqid) +{ + reqid_entry_t *entry, tmpl = { + .reqid = reqid, + }; + + this->mutex->lock(this->mutex); + entry = this->reqids->get(this->reqids, &tmpl); + if (entry) + { + entry->refs++; + } + this->mutex->unlock(this->mutex); + return entry ? SUCCESS : NOT_FOUND; +} + METHOD(kernel_interface_t, release_reqid, status_t, private_kernel_interface_t *this, uint32_t reqid) { @@ -1039,6 +1056,7 @@ kernel_interface_t *kernel_interface_create() .get_spi = _get_spi, .get_cpi = _get_cpi, .alloc_reqid = _alloc_reqid, + .ref_reqid = _ref_reqid, .release_reqid = _release_reqid, .add_sa = _add_sa, .update_sa = _update_sa, diff --git a/src/libcharon/kernel/kernel_interface.h b/src/libcharon/kernel/kernel_interface.h index 9f8e9b714..a70e7f860 100644 --- a/src/libcharon/kernel/kernel_interface.h +++ b/src/libcharon/kernel/kernel_interface.h @@ -158,6 +158,18 @@ struct kernel_interface_t { uint32_t if_id_out, sec_label_t *label, uint32_t *reqid); + /** + * Increase the reference count for the given reqid that was previously + * allocated by alloc_reqid(). + * + * The reference must be released with a call to release_reqid(). + * + * @param reqid previously allocated reqid + * @return SUCCESS if refcount increased, NOT_FOUND if reqid is + * unknown (shouldn't happen) + */ + status_t (*ref_reqid)(kernel_interface_t *this, uint32_t reqid); + /** * Release a previously allocated reqid. * From 13771206d430e0bf31409a14b2c4a0074286598d Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Mon, 2 Oct 2023 15:47:02 +0200 Subject: [PATCH 04/14] child-sa: Keep a reference to the previous reqid The reference is kept until the reqid is either confirmed (i.e. re-allocated) or replaced by a different reqid, which happens only once we know the final traffic selectors, or the SA is destroyed without installing it. --- src/libcharon/sa/child_sa.c | 81 ++++++++++++++++++++++++------------- 1 file changed, 53 insertions(+), 28 deletions(-) diff --git a/src/libcharon/sa/child_sa.c b/src/libcharon/sa/child_sa.c index e23accf0e..9e728bd45 100644 --- a/src/libcharon/sa/child_sa.c +++ b/src/libcharon/sa/child_sa.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2006-2019 Tobias Brunner + * Copyright (C) 2006-2023 Tobias Brunner * Copyright (C) 2016 Andreas Steffen * Copyright (C) 2005-2008 Martin Willi * Copyright (C) 2006 Daniel Roethlisberger @@ -842,6 +842,51 @@ METHOD(child_sa_t, alloc_cpi, uint16_t, return 0; } +/** + * Allocate a reqid for the given local and remote traffic selector lists. + * On success, release the previously allocated reqid. + */ +static status_t alloc_reqid_lists(private_child_sa_t *this, + linked_list_t *my_ts, linked_list_t *other_ts, + uint32_t *reqid) +{ + uint32_t existing_reqid = *reqid; + status_t status; + + status = charon->kernel->alloc_reqid( + charon->kernel, my_ts, other_ts, + this->mark_in, this->mark_out, this->if_id_in, + this->if_id_out, label_for(this, LABEL_USE_REQID), + reqid); + + if (status == SUCCESS && existing_reqid) + { + if (charon->kernel->release_reqid(charon->kernel, + existing_reqid) != SUCCESS) + { + DBG1(DBG_CHD, "releasing previous reqid %u failed", existing_reqid); + } + } + return status; +} + +/** + * Allocate a reqid for the given local and remote traffic selectors. + */ +static status_t alloc_reqid(private_child_sa_t *this, array_t *my_ts, + array_t *other_ts, uint32_t *reqid) +{ + linked_list_t *my_ts_list, *other_ts_list; + status_t status; + + my_ts_list = linked_list_create_from_enumerator(array_create_enumerator(my_ts)); + other_ts_list = linked_list_create_from_enumerator(array_create_enumerator(other_ts)); + status = alloc_reqid_lists(this, my_ts_list, other_ts_list, reqid); + my_ts_list->destroy(my_ts_list); + other_ts_list->destroy(other_ts_list); + return status; +} + /** * Install the given SA in the kernel */ @@ -923,10 +968,7 @@ static status_t install_internal(private_child_sa_t *this, chunk_t encr, if (!this->reqid_allocated && !this->static_reqid) { - status = charon->kernel->alloc_reqid(charon->kernel, my_ts, other_ts, - this->mark_in, this->mark_out, this->if_id_in, - this->if_id_out, label_for(this, LABEL_USE_REQID), - &this->reqid); + status = alloc_reqid_lists(this, my_ts, other_ts, &this->reqid); if (status != SUCCESS) { my_ts->destroy(my_ts); @@ -1321,27 +1363,6 @@ METHOD(child_sa_t, set_policies, void, array_sort(this->other_ts, (void*)traffic_selector_cmp, NULL); } -/** - * Allocate a reqid for the given local and remote traffic selectors. - */ -static status_t alloc_reqid(private_child_sa_t *this, array_t *my_ts, - array_t *other_ts, uint32_t *reqid) -{ - linked_list_t *my_ts_list, *other_ts_list; - status_t status; - - my_ts_list = linked_list_create_from_enumerator(array_create_enumerator(my_ts)); - other_ts_list = linked_list_create_from_enumerator(array_create_enumerator(other_ts)); - status = charon->kernel->alloc_reqid( - charon->kernel, my_ts_list, other_ts_list, - this->mark_in, this->mark_out, this->if_id_in, - this->if_id_out, label_for(this, LABEL_USE_REQID), - reqid); - my_ts_list->destroy(my_ts_list); - other_ts_list->destroy(other_ts_list); - return status; -} - METHOD(child_sa_t, install_policies, status_t, private_child_sa_t *this) { @@ -1943,7 +1964,7 @@ METHOD(child_sa_t, destroy, void, charon->kernel->del_sa(charon->kernel, &id, &sa); } - if (this->reqid_allocated) + if (this->reqid_allocated || (!this->static_reqid && this->reqid)) { if (charon->kernel->release_reqid(charon->kernel, this->reqid) != SUCCESS) @@ -2126,7 +2147,11 @@ child_sa_t *child_sa_create(host_t *me, host_t *other, child_cfg_t *config, * replace the temporary SA on the kernel level. Rekeying such an SA * requires an explicit reqid, as the cache currently knows the original * selectors only for that reqid. */ - this->reqid = data->reqid; + if (data->reqid && + charon->kernel->ref_reqid(charon->kernel, data->reqid) == SUCCESS) + { + this->reqid = data->reqid; + } } else { From ff269f7f1f6fbab4cbce061fd4f55b0976a14128 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Mon, 2 Oct 2023 15:08:49 +0200 Subject: [PATCH 05/14] child-sa: Add method that returns a reference to an allocated reqid --- src/libcharon/sa/child_sa.c | 12 ++++++++++++ src/libcharon/sa/child_sa.h | 14 +++++++++++++- 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/libcharon/sa/child_sa.c b/src/libcharon/sa/child_sa.c index 9e728bd45..494c36b2e 100644 --- a/src/libcharon/sa/child_sa.c +++ b/src/libcharon/sa/child_sa.c @@ -361,6 +361,17 @@ METHOD(child_sa_t, get_reqid, uint32_t, return this->reqid; } +METHOD(child_sa_t, get_reqid_ref, uint32_t, + private_child_sa_t *this) +{ + if ((this->reqid_allocated || (!this->static_reqid && this->reqid)) && + charon->kernel->ref_reqid(charon->kernel, this->reqid) == SUCCESS) + { + return this->reqid; + } + return 0; +} + METHOD(child_sa_t, get_unique_id, uint32_t, private_child_sa_t *this) { @@ -2033,6 +2044,7 @@ child_sa_t *child_sa_create(host_t *me, host_t *other, child_cfg_t *config, .public = { .get_name = _get_name, .get_reqid = _get_reqid, + .get_reqid_ref = _get_reqid_ref, .get_unique_id = _get_unique_id, .get_config = _get_config, .get_state = _get_state, diff --git a/src/libcharon/sa/child_sa.h b/src/libcharon/sa/child_sa.h index 37f00277e..0b7d11114 100644 --- a/src/libcharon/sa/child_sa.h +++ b/src/libcharon/sa/child_sa.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2006-2019 Tobias Brunner + * Copyright (C) 2006-2023 Tobias Brunner * Copyright (C) 2006-2008 Martin Willi * Copyright (C) 2006 Daniel Roethlisberger * @@ -174,6 +174,18 @@ struct child_sa_t { */ uint32_t (*get_reqid)(child_sa_t *this); + /** + * Get an additional reference to the allocated reqid of this CHILD SA. + * + * For static reqids or until the reqid is allocated (if none was passed + * in the constructor), this returns 0. The returned reqid must be released + * via kernel_interface_t::release_reqid(). + * + * @return allocated reqid of the CHILD SA, 0 if reqid is static or + * not allocated yet + */ + uint32_t (*get_reqid_ref)(child_sa_t *this); + /** * Get the unique numerical identifier for this CHILD_SA. * From 4bfd93b8dbb5ce573b80eb4fd6477205cc9e5327 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Mon, 2 Oct 2023 16:00:07 +0200 Subject: [PATCH 06/14] child-create: Maintain reference to reqid while CHILD_SA is established --- src/libcharon/sa/ikev2/tasks/child_create.c | 15 ++++++++++++++- src/libcharon/sa/ikev2/tasks/child_create.h | 4 ++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/libcharon/sa/ikev2/tasks/child_create.c b/src/libcharon/sa/ikev2/tasks/child_create.c index 928264820..a40941e66 100644 --- a/src/libcharon/sa/ikev2/tasks/child_create.c +++ b/src/libcharon/sa/ikev2/tasks/child_create.c @@ -1925,7 +1925,16 @@ METHOD(task_t, process_i, status_t, METHOD(child_create_t, use_reqid, void, private_child_create_t *this, uint32_t reqid) { - this->child.reqid = reqid; + uint32_t existing_reqid = this->child.reqid; + + if (!reqid || charon->kernel->ref_reqid(charon->kernel, reqid) == SUCCESS) + { + this->child.reqid = reqid; + if (existing_reqid) + { + charon->kernel->release_reqid(charon->kernel, existing_reqid); + } + } } METHOD(child_create_t, use_marks, void, @@ -2064,6 +2073,10 @@ METHOD(task_t, destroy, void, { DESTROY_IF(this->child_sa); } + if (this->child.reqid) + { + charon->kernel->release_reqid(charon->kernel, this->child.reqid); + } DESTROY_IF(this->packet_tsi); DESTROY_IF(this->packet_tsr); DESTROY_IF(this->proposal); diff --git a/src/libcharon/sa/ikev2/tasks/child_create.h b/src/libcharon/sa/ikev2/tasks/child_create.h index 705b7e116..62de4c686 100644 --- a/src/libcharon/sa/ikev2/tasks/child_create.h +++ b/src/libcharon/sa/ikev2/tasks/child_create.h @@ -49,6 +49,10 @@ struct child_create_t { * When this task is used for rekeying, the same reqid is used * for the new CHILD_SA. * + * This must only be called with dynamically allocated reqids (i.e. from + * kernel_interface_t::alloc_reqid()), the method takes a reference that's + * maintained for the lifetime of the task. + * * @param reqid reqid to use */ void (*use_reqid) (child_create_t *this, uint32_t reqid); From 4ea739baf4150025d76de1aa7cfe92bb0659d9f1 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Mon, 2 Oct 2023 16:20:01 +0200 Subject: [PATCH 07/14] quick-mode: Maintain reference to reqid while CHILD_SA is established --- src/libcharon/sa/ikev1/tasks/quick_mode.c | 15 ++++++++++++++- src/libcharon/sa/ikev1/tasks/quick_mode.h | 4 ++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/libcharon/sa/ikev1/tasks/quick_mode.c b/src/libcharon/sa/ikev1/tasks/quick_mode.c index 3d2e3efbe..e403c366d 100644 --- a/src/libcharon/sa/ikev1/tasks/quick_mode.c +++ b/src/libcharon/sa/ikev1/tasks/quick_mode.c @@ -1432,7 +1432,16 @@ METHOD(quick_mode_t, get_mid, uint32_t, METHOD(quick_mode_t, use_reqid, void, private_quick_mode_t *this, uint32_t reqid) { - this->child.reqid = reqid; + uint32_t existing_reqid = this->child.reqid; + + if (!reqid || charon->kernel->ref_reqid(charon->kernel, reqid) == SUCCESS) + { + this->child.reqid = reqid; + if (existing_reqid) + { + charon->kernel->release_reqid(charon->kernel, existing_reqid); + } + } } METHOD(quick_mode_t, use_marks, void, @@ -1496,6 +1505,10 @@ METHOD(task_t, destroy, void, DESTROY_IF(this->child_sa); DESTROY_IF(this->config); DESTROY_IF(this->dh); + if (this->child.reqid) + { + charon->kernel->release_reqid(charon->kernel, this->child.reqid); + } free(this); } diff --git a/src/libcharon/sa/ikev1/tasks/quick_mode.h b/src/libcharon/sa/ikev1/tasks/quick_mode.h index 08aa15f46..2b0489407 100644 --- a/src/libcharon/sa/ikev1/tasks/quick_mode.h +++ b/src/libcharon/sa/ikev1/tasks/quick_mode.h @@ -50,6 +50,10 @@ struct quick_mode_t { /** * Use a specific reqid to install this CHILD_SA. * + * This must only be called with dynamically allocated reqids (i.e. from + * kernel_interface_t::alloc_reqid()), the method takes a reference that's + * maintained for the lifetime of the task. + * * @param reqid reqid to use */ void (*use_reqid)(quick_mode_t *this, uint32_t reqid); From bc39a3aecb37fb8829b8378fdeb58d5778bb0d52 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Mon, 2 Oct 2023 16:02:08 +0200 Subject: [PATCH 08/14] child-rekey: Only set reqid on new CHILD_SA if it was allocated dynamically Keeping a reference ensures that if the old SA expires before the new one is installed, the previous reqid isn't reallocated to a concurrently established CHILD_SA with different selectors. --- src/libcharon/sa/ikev2/tasks/child_rekey.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/libcharon/sa/ikev2/tasks/child_rekey.c b/src/libcharon/sa/ikev2/tasks/child_rekey.c index 124f9b337..3ef175891 100644 --- a/src/libcharon/sa/ikev2/tasks/child_rekey.c +++ b/src/libcharon/sa/ikev2/tasks/child_rekey.c @@ -213,8 +213,12 @@ METHOD(task_t, build_i, status_t, this->child_create->use_dh_group(this->child_create, dh_group); } } - reqid = this->child_sa->get_reqid(this->child_sa); - this->child_create->use_reqid(this->child_create, reqid); + reqid = this->child_sa->get_reqid_ref(this->child_sa); + if (reqid) + { + this->child_create->use_reqid(this->child_create, reqid); + charon->kernel->release_reqid(charon->kernel, reqid); + } this->child_create->use_marks(this->child_create, this->child_sa->get_mark(this->child_sa, TRUE).value, this->child_sa->get_mark(this->child_sa, FALSE).value); @@ -282,8 +286,12 @@ METHOD(task_t, build_r, status_t, } /* let the CHILD_CREATE task build the response */ - reqid = this->child_sa->get_reqid(this->child_sa); - this->child_create->use_reqid(this->child_create, reqid); + reqid = this->child_sa->get_reqid_ref(this->child_sa); + if (reqid) + { + this->child_create->use_reqid(this->child_create, reqid); + charon->kernel->release_reqid(charon->kernel, reqid); + } this->child_create->use_marks(this->child_create, this->child_sa->get_mark(this->child_sa, TRUE).value, this->child_sa->get_mark(this->child_sa, FALSE).value); @@ -430,7 +438,7 @@ METHOD(task_t, process_i, status_t, protocol = this->child_sa->get_protocol(this->child_sa); child_cfg = this->child_sa->get_config(this->child_sa); child_cfg->get_ref(child_cfg); - args.reqid = this->child_sa->get_reqid(this->child_sa); + args.reqid = this->child_sa->get_reqid_ref(this->child_sa); args.label = this->child_sa->get_label(this->child_sa); if (args.label) { @@ -440,6 +448,10 @@ METHOD(task_t, process_i, status_t, this->ike_sa->destroy_child_sa(this->ike_sa, protocol, spi); status = this->ike_sa->initiate(this->ike_sa, child_cfg->get_ref(child_cfg), &args); + if (args.reqid) + { + charon->kernel->release_reqid(charon->kernel, args.reqid); + } DESTROY_IF(args.label); return status; } From f2bc526dbbfb02d28b0ad734de8a8d56b622f593 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Mon, 2 Oct 2023 16:06:48 +0200 Subject: [PATCH 09/14] ikev1: Only set reqid on rekeyed CHILD_SA if it was allocated dynamically --- src/libcharon/sa/ikev1/task_manager_v1.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/libcharon/sa/ikev1/task_manager_v1.c b/src/libcharon/sa/ikev1/task_manager_v1.c index 13e908144..293a8c226 100644 --- a/src/libcharon/sa/ikev1/task_manager_v1.c +++ b/src/libcharon/sa/ikev1/task_manager_v1.c @@ -1791,6 +1791,7 @@ METHOD(task_manager_t, queue_child_rekey, void, child_sa_t *child_sa; child_cfg_t *cfg; quick_mode_t *task; + uint32_t reqid; child_sa = this->ike_sa->get_child_sa(this->ike_sa, protocol, spi, TRUE); if (!child_sa) @@ -1816,7 +1817,12 @@ METHOD(task_manager_t, queue_child_rekey, void, cfg = child_sa->get_config(child_sa); task = quick_mode_create(this->ike_sa, cfg->get_ref(cfg), get_first_ts(child_sa, TRUE), get_first_ts(child_sa, FALSE)); - task->use_reqid(task, child_sa->get_reqid(child_sa)); + reqid = child_sa->get_reqid_ref(child_sa); + if (reqid) + { + task->use_reqid(task, reqid); + charon->kernel->release_reqid(charon->kernel, reqid); + } task->use_marks(task, child_sa->get_mark(child_sa, TRUE).value, child_sa->get_mark(child_sa, FALSE).value); task->use_if_ids(task, child_sa->get_if_id(child_sa, TRUE), From c2a4c8e38a2786013e89b7bd183f6518c6aceb5a Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Mon, 2 Oct 2023 16:21:28 +0200 Subject: [PATCH 10/14] ike-sa: Correctly maintain allocated reqid when recreating CHILD_SA Maintaining the reqid when recreating a CHILD_SA from scratch night not strictly be necessary as we usually don't have to replace any temporary states in the kernel. However, there could be concurrent acquires that might actually make it necessary (we use the reqid to keep track of acquires and it's also part of the duplicate check). --- src/libcharon/sa/ike_sa.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libcharon/sa/ike_sa.c b/src/libcharon/sa/ike_sa.c index 6b0e72da3..654508e4b 100644 --- a/src/libcharon/sa/ike_sa.c +++ b/src/libcharon/sa/ike_sa.c @@ -2067,7 +2067,7 @@ static status_t reestablish_children(private_ike_sa_t *this, ike_sa_t *new, if (action & ACTION_START) { child_init_args_t args = { - .reqid = child_sa->get_reqid(child_sa), + .reqid = child_sa->get_reqid_ref(child_sa), .label = child_sa->get_label(child_sa), }; child_cfg = child_sa->get_config(child_sa); @@ -2076,6 +2076,10 @@ static status_t reestablish_children(private_ike_sa_t *this, ike_sa_t *new, other->task_manager->queue_child(other->task_manager, child_cfg->get_ref(child_cfg), &args); + if (args.reqid) + { + charon->kernel->release_reqid(charon->kernel, args.reqid); + } } } enumerator->destroy(enumerator); From c923022733383a9d1ecbddf3b8ff3eeddd91c050 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Mon, 2 Oct 2023 16:08:40 +0200 Subject: [PATCH 11/14] child-delete: Correctly maintain allocated reqid when recreating CHILD_SA The old CHILD_SA is destroyed even before the new task is queued, this makes sure we always maintain a reference to the reqid. --- src/libcharon/sa/ikev2/tasks/child_delete.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libcharon/sa/ikev2/tasks/child_delete.c b/src/libcharon/sa/ikev2/tasks/child_delete.c index 54aba2c50..2e2668bbe 100644 --- a/src/libcharon/sa/ikev2/tasks/child_delete.c +++ b/src/libcharon/sa/ikev2/tasks/child_delete.c @@ -366,7 +366,7 @@ static status_t destroy_and_reestablish(private_child_delete_t *this) spi = child_sa->get_spi(child_sa, TRUE); child_cfg = child_sa->get_config(child_sa); child_cfg->get_ref(child_cfg); - args.reqid = child_sa->get_reqid(child_sa); + args.reqid = child_sa->get_reqid_ref(child_sa); args.label = child_sa->get_label(child_sa); if (args.label) { @@ -391,6 +391,10 @@ static status_t destroy_and_reestablish(private_child_delete_t *this) } } child_cfg->destroy(child_cfg); + if (args.reqid) + { + charon->kernel->release_reqid(charon->kernel, args.reqid); + } DESTROY_IF(args.label); if (status != SUCCESS) { From f9a9188a36e1b13a3c79cf9dc314cdbebad6625c Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Mon, 2 Oct 2023 16:10:40 +0200 Subject: [PATCH 12/14] quick-delete: Correctly maintain allocated reqid when recreating CHILD_SA --- src/libcharon/sa/ikev1/tasks/quick_delete.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libcharon/sa/ikev1/tasks/quick_delete.c b/src/libcharon/sa/ikev1/tasks/quick_delete.c index bff7a1d7c..0da50ed40 100644 --- a/src/libcharon/sa/ikev1/tasks/quick_delete.c +++ b/src/libcharon/sa/ikev1/tasks/quick_delete.c @@ -151,7 +151,7 @@ static status_t delete_child(private_quick_delete_t *this, if (remote_close) { child_init_args_t args = { - .reqid = child_sa->get_reqid(child_sa), + .reqid = child_sa->get_reqid_ref(child_sa), }; action_t action; @@ -169,6 +169,10 @@ static status_t delete_child(private_quick_delete_t *this, child_cfg->get_ref(child_cfg); status = this->ike_sa->initiate(this->ike_sa, child_cfg, &args); } + if (args.reqid) + { + charon->kernel->release_reqid(charon->kernel, args.reqid); + } child_cfg->destroy(child_cfg); } } From 04bfe83f71ea31d9d9d630120678821301272a87 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Mon, 2 Oct 2023 16:17:33 +0200 Subject: [PATCH 13/14] trap-manager: Maintain allocated reqids when handling acquires --- src/libcharon/sa/trap_manager.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/libcharon/sa/trap_manager.c b/src/libcharon/sa/trap_manager.c index d8d8a421a..1b85c66a5 100644 --- a/src/libcharon/sa/trap_manager.c +++ b/src/libcharon/sa/trap_manager.c @@ -524,6 +524,7 @@ METHOD(trap_manager_t, acquire, void, child_cfg_t *child; ike_sa_t *ike_sa; host_t *host; + uint32_t allocated_reqid; bool wildcard, ignore = FALSE; this->lock->read_lock(this->lock); @@ -596,6 +597,8 @@ METHOD(trap_manager_t, acquire, void, peer = found->peer_cfg->get_ref(found->peer_cfg); child = found->child_sa->get_config(found->child_sa); child = child->get_ref(child); + /* only pass allocated reqids explicitly, take a reference */ + allocated_reqid = found->child_sa->get_reqid_ref(found->child_sa); /* don't hold the lock while checking out the IKE_SA */ this->lock->unlock(this->lock); @@ -635,7 +638,7 @@ METHOD(trap_manager_t, acquire, void, if (ike_sa) { child_init_args_t args = { - .reqid = reqid, + .reqid = allocated_reqid, .src = data->src, .dst = data->dst, .label = data->label, @@ -669,6 +672,10 @@ METHOD(trap_manager_t, acquire, void, destroy_acquire(acquire); child->destroy(child); } + if (allocated_reqid) + { + charon->kernel->release_reqid(charon->kernel, allocated_reqid); + } } /** From 00d054aae53c2835d50241f981c265b0bba9c29e Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Mon, 2 Oct 2023 16:24:01 +0200 Subject: [PATCH 14/14] ikev2: Correctly maintain allocated reqid during make-before-break reauth --- src/libcharon/sa/ikev2/task_manager_v2.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/libcharon/sa/ikev2/task_manager_v2.c b/src/libcharon/sa/ikev2/task_manager_v2.c index 978ea4120..8c07cf272 100644 --- a/src/libcharon/sa/ikev2/task_manager_v2.c +++ b/src/libcharon/sa/ikev2/task_manager_v2.c @@ -2126,6 +2126,7 @@ static void trigger_mbb_reauth(private_task_manager_t *this) ike_sa_t *new; host_t *host; queued_task_t *queued; + uint32_t reqid; bool children = FALSE; new = charon->ike_sa_manager->create_new(charon->ike_sa_manager, @@ -2165,7 +2166,12 @@ static void trigger_mbb_reauth(private_task_manager_t *this) cfg = child_sa->get_config(child_sa); child_create = child_create_create(new, cfg->get_ref(cfg), FALSE, NULL, NULL); - child_create->use_reqid(child_create, child_sa->get_reqid(child_sa)); + reqid = child_sa->get_reqid_ref(child_sa); + if (reqid) + { + child_create->use_reqid(child_create, reqid); + charon->kernel->release_reqid(charon->kernel, reqid); + } child_create->use_marks(child_create, child_sa->get_mark(child_sa, TRUE).value, child_sa->get_mark(child_sa, FALSE).value);