From fefea487248c78b4dbb3c33ea0630ba74921758f Mon Sep 17 00:00:00 2001 From: Thomas Egerer Date: Mon, 25 Nov 2024 15:37:40 +0000 Subject: [PATCH] child-sa: Update status flags based on success of SA addition Both variables `inbound_installed` and `outbound_state` are used in `child_sa_t::destroy()` to determine whether inbound and outbound state have to be deleted. They are assigned prior to the call to `kernel_interface_t::add_sa()`. As this call may fail, the destructor may try to delete a state which it has not been added. By making the assignment of these variables dependent on the success of the state addition, we can make sure, a `child_sa_t::destroy()` only deletes states it has added. Also removed the redundant checks for `my_spi` and `other_spi` being set along with the check for the above flags. It seems that when the flags are set, the SPIs *must* be set. Signed-off-by: Thomas Egerer --- src/libcharon/sa/child_sa.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/src/libcharon/sa/child_sa.c b/src/libcharon/sa/child_sa.c index 1f50c49520..7262342075 100644 --- a/src/libcharon/sa/child_sa.c +++ b/src/libcharon/sa/child_sa.c @@ -620,7 +620,7 @@ static status_t update_usebytes(private_child_sa_t *this, bool inbound) if (inbound) { - if (this->my_spi && this->inbound_installed) + if (this->inbound_installed) { kernel_ipsec_sa_id_t id = { .src = this->other_addr, @@ -654,7 +654,7 @@ static status_t update_usebytes(private_child_sa_t *this, bool inbound) } else { - if (this->other_spi && (this->outbound_state & CHILD_OUTBOUND_SA)) + if (this->outbound_state & CHILD_OUTBOUND_SA) { kernel_ipsec_sa_id_t id = { .src = this->my_addr, @@ -939,7 +939,6 @@ static status_t install_internal(private_child_sa_t *this, chunk_t encr, this->my_cpi = cpi; dst_ts = my_ts; src_ts = other_ts; - this->inbound_installed = TRUE; } else { @@ -954,7 +953,6 @@ static status_t install_internal(private_child_sa_t *this, chunk_t encr, { tfc = this->config->get_tfc(this->config); } - this->outbound_state |= CHILD_OUTBOUND_SA; } DBG2(DBG_CHD, "adding %s %N SA", inbound ? "inbound" : "outbound", @@ -1061,6 +1059,17 @@ static status_t install_internal(private_child_sa_t *this, chunk_t encr, other_ts->destroy(other_ts); free(lifetime); + if (status == SUCCESS) + { + if (inbound) + { + this->inbound_installed = TRUE; + } + else + { + this->outbound_state |= CHILD_OUTBOUND_SA; + } + } return status; } @@ -1623,7 +1632,7 @@ static status_t update_sas(private_child_sa_t *this, host_t *me, host_t *other, bool encap, uint32_t reqid) { /* update our (initiator) SA */ - if (this->my_spi && this->inbound_installed) + if (this->inbound_installed) { kernel_ipsec_sa_id_t id = { .src = this->other_addr, @@ -1649,7 +1658,7 @@ static status_t update_sas(private_child_sa_t *this, host_t *me, host_t *other, } /* update his (responder) SA */ - if (this->other_spi && (this->outbound_state & CHILD_OUTBOUND_SA)) + if (this->outbound_state & CHILD_OUTBOUND_SA) { kernel_ipsec_sa_id_t id = { .src = this->my_addr, @@ -1960,7 +1969,7 @@ METHOD(child_sa_t, destroy, void, }; charon->kernel->del_sa(charon->kernel, &id, &sa); } - if (this->other_spi && (this->outbound_state & CHILD_OUTBOUND_SA)) + if (this->outbound_state & CHILD_OUTBOUND_SA) { kernel_ipsec_sa_id_t id = { .src = this->my_addr, -- 2.47.2