From 2da99e50722f258618fa5fe53f93e603c97d4fe3 Mon Sep 17 00:00:00 2001 From: Gabriel Oginski Date: Tue, 14 Feb 2023 08:46:36 +0000 Subject: vpp-swan: fix memory leaks This patch fix the memory leaks discovered in the current implementation, inlcuding expired data, spd dump, and host names. Type: fix Signed-off-by: Gabriel Oginski Change-Id: I3794f5db3c58d1e78df25f242c91e7a67363de53 --- extras/strongswan/vpp_sswan/kernel_vpp_ipsec.c | 270 +++++++++++++++++++------ extras/strongswan/vpp_sswan/kernel_vpp_net.c | 8 + 2 files changed, 213 insertions(+), 65 deletions(-) (limited to 'extras/strongswan') diff --git a/extras/strongswan/vpp_sswan/kernel_vpp_ipsec.c b/extras/strongswan/vpp_sswan/kernel_vpp_ipsec.c index a51edcbc247..652de6552f7 100644 --- a/extras/strongswan/vpp_sswan/kernel_vpp_ipsec.c +++ b/extras/strongswan/vpp_sswan/kernel_vpp_ipsec.c @@ -62,6 +62,14 @@ #define PRIO_BASE 384 +/** + * Every 2 seconds, the thread responsible for collecting the available + * interfaces will be executed. + * Retrying 5 times every 1 second ensures that there is enough time to check + * if the interface will be available. + */ +#define N_RETRY_GET_IF 5 + u32 natt_port; /** @@ -133,6 +141,11 @@ struct private_kernel_vpp_ipsec_t * route-based IPsec */ bool use_tunnel_mode_sa; + + /** + * Connections to VPP Stats + */ + stat_client_main_t *sm; }; /** @@ -143,6 +156,7 @@ typedef struct /** VPP SA ID */ uint32_t sa_id; uint32_t stat_index; + kernel_ipsec_sa_id_t *sa_id_p; } sa_t; /** @@ -156,6 +170,8 @@ typedef struct uint32_t sw_if_index; /** Policy count for this SPD */ refcount_t policy_num; + /** Name of the interface the SPD is bound to */ + char *if_name; } spd_t; /** @@ -413,9 +429,9 @@ static void manage_route (private_kernel_vpp_ipsec_t *this, bool add, traffic_selector_t *dst_ts, host_t *src, host_t *dst) { - host_t *dst_net, *gateway; + host_t *dst_net = NULL, *gateway = NULL; uint8_t prefixlen; - char *if_name; + char *if_name = NULL; route_entry_t *route; bool route_exist = FALSE; @@ -433,11 +449,11 @@ manage_route (private_kernel_vpp_ipsec_t *this, bool add, { if (src->is_anyaddr (src)) { - return; + goto error; } if (!charon->kernel->get_interface (charon->kernel, src, &if_name)) { - return; + goto error; } } route_exist = @@ -478,7 +494,7 @@ manage_route (private_kernel_vpp_ipsec_t *this, bool add, if (!route_exist) { DBG2 (DBG_KNL, "del route but it not exist"); - return; + goto error; } if (ref_put (&route->refs)) { @@ -489,6 +505,14 @@ manage_route (private_kernel_vpp_ipsec_t *this, bool add, gateway, dst, if_name, 1); } } +error: + if (gateway != NULL) + gateway->destroy (gateway); + if (dst_net != NULL) + dst_net->destroy (dst_net); + if (if_name != NULL) + free (if_name); + return; } /** @@ -796,6 +820,12 @@ bypass_all (bool add, uint32_t spd_id, uint32_t sa_id) ntohl (rmp->retval)); goto error; } + /* address "out" needs to be freed after vec->send */ + if (out != NULL) + { + free (out); + out = NULL; + } mp->entry.is_outbound = 1; if (vac->send (vac, (char *) mp, sizeof (*mp), &out, &out_len)) { @@ -809,6 +839,12 @@ bypass_all (bool add, uint32_t spd_id, uint32_t sa_id) ntohl (rmp->retval)); goto error; } + /* address "out" needs to be freed after vec->send */ + if (out != NULL) + { + free (out); + out = NULL; + } mp->entry.is_outbound = 0; mp->entry.protocol = IP_API_PROTO_AH; if (vac->send (vac, (char *) mp, sizeof (*mp), &out, &out_len)) @@ -823,6 +859,12 @@ bypass_all (bool add, uint32_t spd_id, uint32_t sa_id) ntohl (rmp->retval)); goto error; } + /* address "out" needs to be freed after vec->send */ + if (out != NULL) + { + free (out); + out = NULL; + } mp->entry.is_outbound = 1; if (vac->send (vac, (char *) mp, sizeof (*mp), &out, &out_len)) { @@ -886,6 +928,12 @@ bypass_port (bool add, uint32_t spd_id, uint32_t sa_id, uint16_t port) ntohl (rmp->retval)); goto error; } + /* address "out" needs to be freed after vec->send */ + if (out != NULL) + { + free (out); + out = NULL; + } mp->entry.is_outbound = 1; if (vac->send (vac, (char *) mp, sizeof (*mp), &out, &out_len)) { @@ -954,64 +1002,68 @@ manage_policy (private_kernel_vpp_ipsec_t *this, bool add, kernel_ipsec_policy_id_t *id, kernel_ipsec_manage_policy_t *data) { - spd_t *spd; + spd_t *spd = NULL; char *out = NULL, *interface = NULL; int out_len; uint32_t sw_if_index, spd_id = ~0, sad_id = ~0; status_t rv = FAILED; uint32_t priority, auto_priority; chunk_t src_from, src_to, dst_from, dst_to; - host_t *src, *dst, *addr; - vl_api_ipsec_spd_entry_add_del_t *mp; - vl_api_ipsec_spd_entry_add_del_reply_t *rmp; + host_t *src = NULL, *dst = NULL, *addr = NULL; + vl_api_ipsec_spd_entry_add_del_t *mp = NULL; + vl_api_ipsec_spd_entry_add_del_reply_t *rmp = NULL; bool n_spd = false; + vl_api_ipsec_spd_dump_t *mp_dump = NULL; + vl_api_ipsec_spd_details_t *rmp_dump = NULL, *tmp = NULL; mp = vl_msg_api_alloc (sizeof (*mp)); memset (mp, 0, sizeof (*mp)); this->mutex->lock (this->mutex); - if (!id->interface) + if (id->dir == POLICY_FWD) { - addr = id->dir == POLICY_IN ? data->dst : data->src; - for (int i = 0; i < 5; i++) + DBG1 (DBG_KNL, "policy FWD interface"); + rv = SUCCESS; + goto error; + } + addr = id->dir == POLICY_IN ? data->dst : data->src; + for (int i = 0; i < N_RETRY_GET_IF; i++) + { + if (!charon->kernel->get_interface (charon->kernel, addr, &interface)) { - if (!charon->kernel->get_interface (charon->kernel, addr, - &interface)) - { - DBG1 (DBG_KNL, "policy no interface %H", addr); - interface = NULL; - sleep (1); - } + DBG1 (DBG_KNL, "policy no interface %H", addr); + free (interface); + interface = NULL; + sleep (1); + } - if (interface) - { - DBG1 (DBG_KNL, "policy have interface %H", addr); - break; - } + if (interface) + { + DBG1 (DBG_KNL, "policy have interface %H", addr); + break; } - if (!interface) - goto error; - id->interface = interface; } + if (!interface) + goto error; DBG2 (DBG_KNL, "manage policy [%s] interface [%s]", add ? "ADD" : "DEL", - id->interface); + interface); - spd = this->spds->get (this->spds, id->interface); + spd = this->spds->get (this->spds, interface); if (!spd) { if (!add) { DBG1 (DBG_KNL, "SPD for %s not found, should not be deleted", - id->interface); + interface); goto error; } - sw_if_index = get_sw_if_index (id->interface); + sw_if_index = get_sw_if_index (interface); DBG1 (DBG_KNL, "firstly created, spd for %s found sw_if_index is %d", - id->interface, sw_if_index); + interface, sw_if_index); if (sw_if_index == ~0) { - DBG1 (DBG_KNL, "sw_if_index for %s not found", id->interface); + DBG1 (DBG_KNL, "sw_if_index for %s not found", interface); goto error; } spd_id = ref_get (&this->next_spd_id); @@ -1026,9 +1078,9 @@ manage_policy (private_kernel_vpp_ipsec_t *this, bool add, sw_if_index); goto error; } - INIT (spd, .spd_id = spd_id, .sw_if_index = sw_if_index, - .policy_num = 0, ); - this->spds->put (this->spds, id->interface, spd); + INIT (spd, .spd_id = spd_id, .sw_if_index = sw_if_index, .policy_num = 0, + .if_name = strdup (interface), ); + this->spds->put (this->spds, spd->if_name, spd); n_spd = true; } @@ -1145,9 +1197,6 @@ manage_policy (private_kernel_vpp_ipsec_t *this, bool add, mp->entry.remote_port_stop = htons (id->dst_ts->get_to_port (id->dst_ts)); /* check if policy exists in SPD */ - vl_api_ipsec_spd_dump_t *mp_dump; - vl_api_ipsec_spd_details_t *rmp_dump, *tmp; - mp_dump = vl_msg_api_alloc (sizeof (*mp_dump)); memset (mp_dump, 0, sizeof (*mp_dump)); @@ -1216,7 +1265,17 @@ next: interface_add_del_spd (FALSE, spd->spd_id, spd->sw_if_index); manage_bypass (FALSE, spd->spd_id, sad_id); spd_add_del (FALSE, spd->spd_id); - this->spds->remove (this->spds, id->interface); + this->spds->remove (this->spds, interface); + if (spd->if_name) + { + free (spd->if_name); + spd->if_name = NULL; + } + if (spd) + { + free (spd); + spd = NULL; + } } } @@ -1229,9 +1288,18 @@ next: } rv = SUCCESS; error: - free (out); - vl_msg_api_free (mp_dump); - vl_msg_api_free (mp); + if (out != NULL) + free (out); + if (mp_dump != NULL) + vl_msg_api_free (mp_dump); + if (mp != NULL) + vl_msg_api_free (mp); + if (src != NULL) + src->destroy (src); + if (dst != NULL) + dst->destroy (dst); + if (interface != NULL) + free (interface); this->mutex->unlock (this->mutex); return rv; } @@ -1275,6 +1343,15 @@ typedef struct } vpp_sa_expired_t; +/** + * Clean up expire data + */ +static void +expire_data_destroy (vpp_sa_expired_t *data) +{ + free (data); +} + /** * Callback for expiration events */ @@ -1294,6 +1371,10 @@ sa_expired (vpp_sa_expired_t *expired) FALSE); } + if (id->src) + id->src->destroy (id->src); + if (id->dst) + id->dst->destroy (id->dst); free (id); this->mutex->unlock (this->mutex); return JOB_REQUEUE_NONE; @@ -1334,8 +1415,9 @@ schedule_expiration (private_kernel_vpp_ipsec_t *this, timeout = lifetime->time.life; } - job = callback_job_create ((callback_job_cb_t) sa_expired, expired, - (callback_job_cleanup_t) free, NULL); + job = + callback_job_create ((callback_job_cb_t) sa_expired, expired, + (callback_job_cleanup_t) expire_data_destroy, NULL); lib->scheduler->schedule_job (lib->scheduler, (job_t *) job, timeout); } @@ -1554,7 +1636,8 @@ METHOD (kernel_ipsec_t, add_sa, status_t, private_kernel_vpp_ipsec_t *this, this->mutex->lock (this->mutex); INIT (sa_id, .src = id->src->clone (id->src), .dst = id->dst->clone (id->dst), .spi = id->spi, .proto = id->proto, ); - INIT (sa, .sa_id = sad_id, .stat_index = ntohl (rmp->stat_index), ); + INIT (sa, .sa_id = sad_id, .stat_index = ntohl (rmp->stat_index), + .sa_id_p = sa_id, ); DBG4 (DBG_KNL, "put sa by its sa_id %x !!!!!!", sad_id); this->sas->put (this->sas, sa_id, sa); schedule_expiration (this, data, id); @@ -1581,31 +1664,48 @@ METHOD (kernel_ipsec_t, query_sa, status_t, private_kernel_vpp_ipsec_t *this, { status_t rv = FAILED; sa_t *sa; - u32 *dir; + u32 *dir = NULL; int i, k; - stat_segment_data_t *res; + stat_segment_data_t *res = NULL; u8 **pattern = 0; uint64_t res_bytes = 0; uint64_t res_packets = 0; this->mutex->lock (this->mutex); sa = this->sas->get (this->sas, id); - this->mutex->unlock (this->mutex); if (!sa) { + this->mutex->unlock (this->mutex); DBG1 (DBG_KNL, "SA not found"); return NOT_FOUND; } - int rv_stat = stat_segment_connect ("/run/vpp/stats.sock"); - if (rv_stat != 0) + if (this->sm == NULL) { - DBG1 (DBG_KNL, "Not connecting with stats segmentation"); - return NOT_FOUND; + stat_client_main_t *sm = NULL; + sm = stat_client_get (); + + if (!sm) + { + DBG1 (DBG_KNL, "Not connecting with stats segmentation"); + this->mutex->unlock (this->mutex); + return NOT_FOUND; + } + this->sm = sm; + int rv_stat = stat_segment_connect_r ("/run/vpp/stats.sock", this->sm); + if (rv_stat != 0) + { + stat_client_free (this->sm); + this->sm = NULL; + DBG1 (DBG_KNL, "Not connecting with stats segmentation"); + this->mutex->unlock (this->mutex); + return NOT_FOUND; + } } + vec_add1 (pattern, (u8 *) "/net/ipsec/sa"); - dir = stat_segment_ls ((u8 **) pattern); - res = stat_segment_dump (dir); + dir = stat_segment_ls_r ((u8 **) pattern, this->sm); + res = stat_segment_dump_r (dir, this->sm); /* i-loop for each results find by pattern - here two: * 1. /net/ipsec/sa * 2. /net/ipsec/sa/lost @@ -1644,11 +1744,10 @@ METHOD (kernel_ipsec_t, query_sa, status_t, private_kernel_vpp_ipsec_t *this, break; } } - stat_segment_data_free (res); - stat_segment_disconnect (); vec_free (pattern); vec_free (dir); + stat_segment_data_free (res); if (bytes) { @@ -1663,6 +1762,7 @@ METHOD (kernel_ipsec_t, query_sa, status_t, private_kernel_vpp_ipsec_t *this, *time = 0; } + this->mutex->unlock (this->mutex); rv = SUCCESS; return rv; } @@ -1672,8 +1772,8 @@ METHOD (kernel_ipsec_t, del_sa, status_t, private_kernel_vpp_ipsec_t *this, { char *out = NULL; int out_len; - vl_api_ipsec_sad_entry_add_del_t *mp; - vl_api_ipsec_sad_entry_add_del_reply_t *rmp; + vl_api_ipsec_sad_entry_add_del_t *mp = NULL; + vl_api_ipsec_sad_entry_add_del_reply_t *rmp = NULL; status_t rv = FAILED; sa_t *sa; @@ -1705,11 +1805,20 @@ METHOD (kernel_ipsec_t, del_sa, status_t, private_kernel_vpp_ipsec_t *this, goto error; } - vl_msg_api_free (mp); - this->sas->remove (this->sas, id); + void *temp = this->sas->remove (this->sas, id); + if (sa->sa_id_p) + { + if (sa->sa_id_p->src) + sa->sa_id_p->src->destroy (sa->sa_id_p->src); + if (sa->sa_id_p->dst) + sa->sa_id_p->dst->destroy (sa->sa_id_p->dst); + free (sa->sa_id_p); + } + free (sa); rv = SUCCESS; error: free (out); + vl_msg_api_free (mp); this->mutex->unlock (this->mutex); return rv; } @@ -1719,12 +1828,14 @@ METHOD (kernel_ipsec_t, flush_sas, status_t, private_kernel_vpp_ipsec_t *this) enumerator_t *enumerator; int out_len; char *out; - vl_api_ipsec_sad_entry_add_del_t *mp; + vl_api_ipsec_sad_entry_add_del_t *mp = NULL; + vl_api_ipsec_sad_entry_add_del_reply_t *rmp = NULL; sa_t *sa = NULL; + status_t rv = FAILED; this->mutex->lock (this->mutex); enumerator = this->sas->create_enumerator (this->sas); - while (enumerator->enumerate (enumerator, sa, NULL)) + while (enumerator->enumerate (enumerator, &sa)) { mp = vl_msg_api_alloc (sizeof (*mp)); memset (mp, 0, sizeof (*mp)); @@ -1736,16 +1847,38 @@ METHOD (kernel_ipsec_t, flush_sas, status_t, private_kernel_vpp_ipsec_t *this) if (vac->send (vac, (char *) mp, sizeof (*mp), &out, &out_len)) { DBG1 (DBG_KNL, "flush_sas failed!!!!"); - return FALSE; + goto error; + } + rmp = (void *) out; + if (rmp->retval) + { + DBG1 (DBG_KNL, "flush_sas failed!!!! rv: %d", ntohl (rmp->retval)); + goto error; + } + if (sa->sa_id_p) + { + if (sa->sa_id_p->src) + sa->sa_id_p->src->destroy (sa->sa_id_p->src); + if (sa->sa_id_p->dst) + sa->sa_id_p->dst->destroy (sa->sa_id_p->dst); } free (out); vl_msg_api_free (mp); this->sas->remove_at (this->sas, enumerator); + free (sa->sa_id_p); + free (sa); } + rv = SUCCESS; +error: + if (out != NULL) + free (out); + if (mp != NULL) + vl_msg_api_free (mp); + enumerator->destroy (enumerator); this->mutex->unlock (this->mutex); - return SUCCESS; + return rv; } METHOD (kernel_ipsec_t, add_policy, status_t, private_kernel_vpp_ipsec_t *this, @@ -1792,6 +1925,12 @@ METHOD (kernel_ipsec_t, destroy, void, private_kernel_vpp_ipsec_t *this) this->sas->destroy (this->sas); this->spds->destroy (this->spds); this->routes->destroy (this->routes); + if (this->sm) + { + stat_segment_disconnect_r (this->sm); + stat_client_free (this->sm); + this->sm = NULL; + } free (this); } @@ -1833,6 +1972,7 @@ kernel_vpp_ipsec_create () .use_tunnel_mode_sa = lib->settings->get_bool(lib->settings, "%s.plugins.kernel-vpp.use_tunnel_mode_sa", TRUE, lib->ns), + .sm = NULL, ); if (!init_spi (this)) diff --git a/extras/strongswan/vpp_sswan/kernel_vpp_net.c b/extras/strongswan/vpp_sswan/kernel_vpp_net.c index a29a7c6a4da..1ed58436286 100644 --- a/extras/strongswan/vpp_sswan/kernel_vpp_net.c +++ b/extras/strongswan/vpp_sswan/kernel_vpp_net.c @@ -545,6 +545,7 @@ update_addrs (private_kernel_vpp_net_t *this, iface_t *entry) vl_api_ip_address_details_t *rmp, *tmp; linked_list_t *addrs; host_t *host; + enumerator_t *enumerator; mp = vl_msg_api_alloc (sizeof (*mp)); clib_memset (mp, 0, sizeof (*mp)); @@ -591,6 +592,13 @@ update_addrs (private_kernel_vpp_net_t *this, iface_t *entry) vl_msg_api_free (mp); free (out); + /* clean-up */ + enumerator = entry->addrs->create_enumerator (entry->addrs); + while (enumerator->enumerate (enumerator, &host)) + { + host->destroy (host); + } + enumerator->destroy (enumerator); entry->addrs->destroy (entry->addrs); entry->addrs = linked_list_create_from_enumerator (addrs->create_enumerator (addrs)); -- cgit 1.2.3-korg