From 81b504228564980bf4dcc7ad4808e2833012c35e Mon Sep 17 00:00:00 2001 From: Rafael Zalamena Date: Fri, 1 May 2020 22:14:00 -0300 Subject: [PATCH] lib: fix route map crash on prefix list removal Changes: - Refactor list entry deletion to use a function that properly notifies route map on deletion (fixes a heap-use-after-free). - Prefix list entry wild card sets `le` to maximum IP mask value and `any` is a boolean. - Fix prefix list trie removal order (in `prefix_list_entry_update_start`). - Let only the `any` callback change the value of field `any`. Signed-off-by: Rafael Zalamena --- lib/filter_nb.c | 41 ++++++++++++++++++++++++++++++++++------- lib/plist.c | 33 ++++++++++++++++++++++++++++++++- lib/plist_int.h | 1 + 3 files changed, 67 insertions(+), 8 deletions(-) diff --git a/lib/filter_nb.c b/lib/filter_nb.c index 54811ac245..9df37d7ccc 100644 --- a/lib/filter_nb.c +++ b/lib/filter_nb.c @@ -98,6 +98,19 @@ prefix_list_length_validate(const struct lyd_node *dnode) return NB_ERR_VALIDATION; } +/** + * Sets prefix list entry to blank value. + * + * \param[out] ple prefix list entry to modify. + */ +static void prefix_list_entry_set_empty(struct prefix_list_entry *ple) +{ + ple->any = false; + memset(&ple->prefix, 0, sizeof(ple->prefix)); + ple->ge = 0; + ple->le = 0; +} + /* * XPath: /frr-filter:lib/access-list-legacy */ @@ -836,8 +849,8 @@ static int lib_prefix_list_entry_create(struct nb_cb_create_args *args) pl = nb_running_get_entry(args->dnode, NULL, true); ple = prefix_list_entry_new(); ple->pl = pl; - ple->any = 1; ple->seq = yang_dnode_get_uint32(args->dnode, "./sequence"); + prefix_list_entry_set_empty(ple); nb_running_set_entry(args->dnode, ple); return NB_OK; @@ -852,7 +865,7 @@ static int lib_prefix_list_entry_destroy(struct nb_cb_destroy_args *args) ple = nb_running_unset_entry(args->dnode); if (ple->installed) - prefix_list_entry_delete(ple->pl, ple, 0); + prefix_list_entry_delete2(ple); else prefix_list_entry_free(ple); @@ -904,7 +917,6 @@ lib_prefix_list_entry_ipv4_prefix_modify(struct nb_cb_modify_args *args) prefix_list_entry_update_start(ple); yang_dnode_get_prefix(&ple->prefix, args->dnode, NULL); - ple->any = 0; /* Finish prefix entry update procedure. */ prefix_list_entry_update_finish(ple); @@ -926,7 +938,6 @@ lib_prefix_list_entry_ipv4_prefix_destroy(struct nb_cb_destroy_args *args) prefix_list_entry_update_start(ple); memset(&ple->prefix, 0, sizeof(ple->prefix)); - ple->any = 1; /* Finish prefix entry update procedure. */ prefix_list_entry_update_finish(ple); @@ -1087,6 +1098,7 @@ static int lib_prefix_list_entry_ipv6_prefix_length_lesser_or_equal_destroy( static int lib_prefix_list_entry_any_create(struct nb_cb_create_args *args) { struct prefix_list_entry *ple; + int type; if (args->event != NB_EV_APPLY) return NB_OK; @@ -1096,8 +1108,24 @@ static int lib_prefix_list_entry_any_create(struct nb_cb_create_args *args) /* Start prefix entry update procedure. */ prefix_list_entry_update_start(ple); + ple->any = true; + + /* Fill prefix struct from scratch. */ memset(&ple->prefix, 0, sizeof(ple->prefix)); - ple->any = 1; + + type = yang_dnode_get_enum(args->dnode, "../../type"); + switch (type) { + case 0: /* ipv4 */ + ple->prefix.family = AF_INET; + ple->ge = 0; + ple->le = IPV4_MAX_BITLEN; + break; + case 1: /* ipv6 */ + ple->prefix.family = AF_INET6; + ple->ge = 0; + ple->le = IPV6_MAX_BITLEN; + break; + } /* Finish prefix entry update procedure. */ prefix_list_entry_update_finish(ple); @@ -1117,8 +1145,7 @@ static int lib_prefix_list_entry_any_destroy(struct nb_cb_destroy_args *args) /* Start prefix entry update procedure. */ prefix_list_entry_update_start(ple); - memset(&ple->prefix, 0, sizeof(ple->prefix)); - ple->any = 1; + prefix_list_entry_set_empty(ple); /* Finish prefix entry update procedure. */ prefix_list_entry_update_finish(ple); diff --git a/lib/plist.c b/lib/plist.c index 958d98dc96..981e86e2ac 100644 --- a/lib/plist.c +++ b/lib/plist.c @@ -661,6 +661,8 @@ void prefix_list_entry_update_start(struct prefix_list_entry *ple) if (!ple->installed) return; + prefix_list_trie_del(pl, ple); + /* List manipulation: shameless copy from `prefix_list_entry_delete`. */ if (ple->prev) ple->prev->next = ple->next; @@ -671,11 +673,17 @@ void prefix_list_entry_update_start(struct prefix_list_entry *ple) else pl->tail = ple->prev; - prefix_list_trie_del(pl, ple); route_map_notify_pentry_dependencies(pl->name, ple, RMAP_EVENT_PLIST_DELETED); pl->count--; + route_map_notify_dependencies(pl->name, RMAP_EVENT_PLIST_DELETED); + if (pl->master->delete_hook) + (*pl->master->delete_hook)(pl); + + if (pl->head || pl->tail || pl->desc) + pl->master->recent = pl; + ple->installed = false; } @@ -695,6 +703,14 @@ void prefix_list_entry_update_finish(struct prefix_list_entry *ple) if (ple->installed) return; + /* + * Check if the entry is installable: + * We can only install entry if at least the prefix is provided (IPv4 + * or IPv6). + */ + if (ple->prefix.family != AF_INET && ple->prefix.family != AF_INET6) + return; + /* List manipulation: shameless copy from `prefix_list_entry_add`. */ if (pl->tail && ple->seq > pl->tail->seq) point = NULL; @@ -742,6 +758,21 @@ void prefix_list_entry_update_finish(struct prefix_list_entry *ple) ple->installed = true; } +/** + * Same as `prefix_list_entry_delete` but without `free()`ing the list if its + * empty. + * + * \param[in] ple prefix list entry. + */ +void prefix_list_entry_delete2(struct prefix_list_entry *ple) +{ + /* Does the boiler plate list removal and entry removal notification. */ + prefix_list_entry_update_start(ple); + + /* Effective `free()` memory. */ + prefix_list_entry_free(ple); +} + /* Return string of prefix_list_type. */ static const char *prefix_list_type_str(struct prefix_list_entry *pentry) { diff --git a/lib/plist_int.h b/lib/plist_int.h index cef78a3ef7..5e0beabbc6 100644 --- a/lib/plist_int.h +++ b/lib/plist_int.h @@ -78,6 +78,7 @@ struct prefix_list_entry { }; extern void prefix_list_entry_free(struct prefix_list_entry *pentry); +extern void prefix_list_entry_delete2(struct prefix_list_entry *ple); extern void prefix_list_entry_update_start(struct prefix_list_entry *ple); extern void prefix_list_entry_update_finish(struct prefix_list_entry *ple); -- 2.39.5