From 29838e7dda08de518b6d37cf2c134f0207334a3a Mon Sep 17 00:00:00 2001 From: Louis Scalbert Date: Fri, 6 Oct 2023 14:16:02 +0200 Subject: [PATCH] bgpd,lib: deal with bgp-flowspec pointer in bgpd BGP-FlowSpec (BGP-FS) utilizes the 'ptr' pointer within the lib to store a raw copy of BGP-FS NLRI as seen in the structure definitions: > struct flowspec_prefix { > [...] > uintptr_t ptr; > }; > > /* FRR generic prefix structure. */ > struct prefix { > uint8_t family; > uint16_t prefixlen; > union { > uint8_t prefix; > [...] > struct linkstate_prefix prefix_flowspec; /* AF_FLOWSPEC */ > [...] > } u __attribute__((aligned(8))); > }; When copying a BGP-FS prefix with prefix_copy(), memory is allocated for a new 'ptr' pointer in the copy. If a 'struct prefix' for BGP-FS is declared on the stack and filled using prefix_copy(), this new pointer must be freed before the function exits. Releasing this pointer is not obvious. Forgetting to do so might leads to memory leaks. For example, the flowspec 'ptr' release is missing in route_node_get(). Deal with BGP-FS 'ptr' pointer memory in bgpd itself. Signed-off-by: Louis Scalbert --- bgpd/bgp_flowspec.c | 7 +------ bgpd/bgp_flowspec_util.c | 20 ++++++++++++++++++++ bgpd/bgp_flowspec_util.h | 2 ++ bgpd/bgp_table.c | 2 ++ bgpd/bgp_table.h | 4 ++++ lib/prefix.c | 23 +++++------------------ 6 files changed, 34 insertions(+), 24 deletions(-) diff --git a/bgpd/bgp_flowspec.c b/bgpd/bgp_flowspec.c index 6165bf892e72..c0c189793750 100644 --- a/bgpd/bgp_flowspec.c +++ b/bgpd/bgp_flowspec.c @@ -90,7 +90,6 @@ int bgp_nlri_parse_flowspec(struct peer *peer, struct attr *attr, safi_t safi; int psize = 0; struct prefix p; - void *temp; /* Start processing the NLRI - there may be multiple in the MP_REACH */ pnt = packet->nlri; @@ -152,9 +151,7 @@ int bgp_nlri_parse_flowspec(struct peer *peer, struct attr *attr, /* Flowspec encoding is in bytes */ p.u.prefix_flowspec.prefixlen = psize; p.u.prefix_flowspec.family = afi2family(afi); - temp = XCALLOC(MTYPE_TMP, psize); - memcpy(temp, pnt, psize); - p.u.prefix_flowspec.ptr = (uintptr_t) temp; + p.u.prefix_flowspec.ptr = bgp_flowspec_ptr_new(pnt, psize); if (BGP_DEBUG(flowspec, FLOWSPEC)) { char return_string[BGP_FLOWSPEC_NLRI_STRING_MAX]; @@ -197,8 +194,6 @@ int bgp_nlri_parse_flowspec(struct peer *peer, struct attr *attr, bgp_withdraw(peer, &p, 0, afi, safi, ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, NULL, NULL, 0, NULL); } - - XFREE(MTYPE_TMP, temp); } return BGP_NLRI_PARSE_OK; } diff --git a/bgpd/bgp_flowspec_util.c b/bgpd/bgp_flowspec_util.c index 66426ab32f02..652c0538fa2c 100644 --- a/bgpd/bgp_flowspec_util.c +++ b/bgpd/bgp_flowspec_util.c @@ -19,6 +19,26 @@ #include "bgp_pbr.h" #include "bgp_errors.h" +DEFINE_MTYPE_STATIC(BGPD, PTR_FLOWSPEC, "FLOWSPEC Prefix Pointer"); + +uintptr_t bgp_flowspec_ptr_new(uint8_t *pnt, uint16_t length) +{ + void *ptr = XCALLOC(MTYPE_PTR_FLOWSPEC, length); + + memcpy(ptr, pnt, length); + + return (uintptr_t)ptr; +} + +void bgp_flowspec_ptr_free(uintptr_t ptr) +{ + void *tmp; + + tmp = (void *)ptr; + + XFREE(MTYPE_PTR_FLOWSPEC, tmp); +} + static void hex2bin(uint8_t *hex, int *bin) { int remainder = *hex; diff --git a/bgpd/bgp_flowspec_util.h b/bgpd/bgp_flowspec_util.h index de1566083873..25153236cf69 100644 --- a/bgpd/bgp_flowspec_util.h +++ b/bgpd/bgp_flowspec_util.h @@ -17,6 +17,8 @@ enum bgp_flowspec_util_nlri_t { BGP_FLOWSPEC_RETURN_JSON = 3, }; +extern uintptr_t bgp_flowspec_ptr_new(uint8_t *pnt, uint16_t length); +extern void bgp_flowspec_ptr_free(uintptr_t ptr); extern int bgp_flowspec_op_decode(enum bgp_flowspec_util_nlri_t type, uint8_t *nlri_ptr, diff --git a/bgpd/bgp_table.c b/bgpd/bgp_table.c index ad5cab67420c..317a85480eaa 100644 --- a/bgpd/bgp_table.c +++ b/bgpd/bgp_table.c @@ -119,6 +119,8 @@ static void bgp_node_destroy(route_table_delegate_t *delegate, if (node->p.family == AF_LINKSTATE) bgp_linkstate_ptr_free(node->p.u.prefix_linkstate.ptr); + else if (node->p.family == AF_FLOWSPEC) + bgp_flowspec_ptr_free(node->p.u.prefix_flowspec.ptr); XFREE(MTYPE_ROUTE_NODE, node); } diff --git a/bgpd/bgp_table.h b/bgpd/bgp_table.h index c55fa60af3c0..f59ba23cfa8a 100644 --- a/bgpd/bgp_table.h +++ b/bgpd/bgp_table.h @@ -12,6 +12,7 @@ #include "linklist.h" #include "bgpd.h" #include "bgp_advertise.h" +#include "bgp_flowspec_util.h" #include "bgp_linkstate.h" struct bgp_table { @@ -242,6 +243,9 @@ static inline struct bgp_dest *bgp_node_get(struct bgp_table *const table, if (p->family == AF_LINKSTATE && table->route_table->count == count) /* ptr pointer has not referenced in a new route_node, free it. */ bgp_linkstate_ptr_free(p->u.prefix_linkstate.ptr); + else if (p->family == AF_FLOWSPEC && table->route_table->count == count) + /* ptr pointer has not referenced in a new route_node, free it. */ + bgp_flowspec_ptr_free(p->u.prefix_flowspec.ptr); if (!rn->info) { struct bgp_dest *dest = XCALLOC(MTYPE_BGP_NODE, diff --git a/lib/prefix.c b/lib/prefix.c index 9bf5ceeda8f0..0a3906b62dd0 100644 --- a/lib/prefix.c +++ b/lib/prefix.c @@ -333,8 +333,6 @@ void prefix_copy(union prefixptr udest, union prefixconstptr usrc) { struct prefix *dest = udest.p; const struct prefix *src = usrc.p; - void *temp; - int len; dest->family = src->family; dest->prefixlen = src->prefixlen; @@ -353,15 +351,11 @@ void prefix_copy(union prefixptr udest, union prefixconstptr usrc) dest->u.lp.id = src->u.lp.id; dest->u.lp.adv_router = src->u.lp.adv_router; } else if (src->family == AF_FLOWSPEC) { - len = src->u.prefix_flowspec.prefixlen; dest->u.prefix_flowspec.prefixlen = src->u.prefix_flowspec.prefixlen; dest->u.prefix_flowspec.family = src->u.prefix_flowspec.family; - temp = XCALLOC(MTYPE_PREFIX_FLOWSPEC, len); - dest->u.prefix_flowspec.ptr = (uintptr_t)temp; - memcpy((void *)dest->u.prefix_flowspec.ptr, - (void *)src->u.prefix_flowspec.ptr, len); + dest->u.prefix_flowspec.ptr = src->u.prefix_flowspec.ptr; } else if (src->family == AF_LINKSTATE) { dest->u.prefix_linkstate.nlri_type = src->u.prefix_linkstate.nlri_type; @@ -1409,8 +1403,6 @@ char *prefix_mac2str(const struct ethaddr *mac, char *buf, int size) unsigned prefix_hash_key(const void *pp) { struct prefix copy; - uint32_t len; - void *temp; /* make sure *all* unused bits are zero, particularly including * alignment / @@ -1418,15 +1410,10 @@ unsigned prefix_hash_key(const void *pp) memset(©, 0, sizeof(copy)); prefix_copy(©, (struct prefix *)pp); - if (((struct prefix *)pp)->family == AF_FLOWSPEC) { - len = jhash((void *)copy.u.prefix_flowspec.ptr, - copy.u.prefix_flowspec.prefixlen, - 0x55aa5a5a); - temp = (void *)copy.u.prefix_flowspec.ptr; - XFREE(MTYPE_PREFIX_FLOWSPEC, temp); - copy.u.prefix_flowspec.ptr = (uintptr_t)NULL; - return len; - } else if (((struct prefix *)pp)->family == AF_LINKSTATE) + if (((struct prefix *)pp)->family == AF_FLOWSPEC) + return jhash((void *)copy.u.prefix_flowspec.ptr, + copy.u.prefix_flowspec.prefixlen, 0x55aa5a5a); + else if (((struct prefix *)pp)->family == AF_LINKSTATE) return jhash((void *)copy.u.prefix_linkstate.ptr, copy.prefixlen, 0x55aa5a5a);