From 5bf15faa19288144c214ca48ef26e01512037ef6 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Mon, 6 Jan 2020 12:58:41 -0500 Subject: [PATCH 1/3] zebra: don't created connected if duplicate depend Since we are using a UNIQUE RB tree, we need to handle the case of adding in a duplicate entry into it. The list API code returns NULL when a successfull add occurs, so lets pull that handling further up into the connected handlers. Then, free the allocated connected struct if it is a duplicate. This is a pretty unlikely situation to happen. Also, pull up the RB handling of _del RB API as well. This was found with the zapi fuzzing code. ``` ==1052840== ==1052840== 200 bytes in 5 blocks are definitely lost in loss record 545 of 663 ==1052840== at 0x483BB1A: calloc (vg_replace_malloc.c:762) ==1052840== by 0x48E1008: qcalloc (memory.c:110) ==1052840== by 0x44D357: nhg_connected_new (zebra_nhg.c:73) ==1052840== by 0x44D300: nhg_connected_tree_add_nhe (zebra_nhg.c:123) ==1052840== by 0x44FBDC: depends_add (zebra_nhg.c:1077) ==1052840== by 0x44FD62: depends_find_add (zebra_nhg.c:1090) ==1052840== by 0x44E46D: zebra_nhg_find (zebra_nhg.c:567) ==1052840== by 0x44E1FE: zebra_nhg_rib_find (zebra_nhg.c:1126) ==1052840== by 0x45AD3D: rib_add_multipath (zebra_rib.c:2616) ==1052840== by 0x4977DC: zread_route_add (zapi_msg.c:1596) ==1052840== by 0x49ABB9: zserv_handle_commands (zapi_msg.c:2636) ==1052840== by 0x428B11: main (main.c:309) ``` Signed-off-by: Stephen Worley --- zebra/zebra_nhg.c | 55 ++++++++++++++++++++++++++++++++------- zebra/zebra_nhg_private.h | 21 ++++++++++++--- 2 files changed, 62 insertions(+), 14 deletions(-) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 7430307320..0925705a25 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -99,31 +99,60 @@ nhg_connected_tree_root(struct nhg_connected_tree_head *head) return nhg_connected_tree_first(head); } -void nhg_connected_tree_del_nhe(struct nhg_connected_tree_head *head, - struct nhg_hash_entry *depend) +struct nhg_hash_entry * +nhg_connected_tree_del_nhe(struct nhg_connected_tree_head *head, + struct nhg_hash_entry *depend) { struct nhg_connected lookup = {}; struct nhg_connected *remove = NULL; + struct nhg_hash_entry *removed_nhe; lookup.nhe = depend; /* Lookup to find the element, then remove it */ remove = nhg_connected_tree_find(head, &lookup); - remove = nhg_connected_tree_del(head, remove); - if (remove) + /* Re-returning here just in case this API changes.. + * the _del list api's are a bit undefined at the moment. + * + * So hopefully returning here will make it fail if the api + * changes to something different than currently expected. + */ + remove = nhg_connected_tree_del(head, remove); + + /* If the entry was sucessfully removed, free the 'connected` struct */ + if (remove) { + removed_nhe = remove->nhe; nhg_connected_free(remove); + return removed_nhe; + } + + return NULL; } -void nhg_connected_tree_add_nhe(struct nhg_connected_tree_head *head, - struct nhg_hash_entry *depend) +/* Assuming UNIQUE RB tree. If this changes, assumptions here about + * insertion need to change. + */ +struct nhg_hash_entry * +nhg_connected_tree_add_nhe(struct nhg_connected_tree_head *head, + struct nhg_hash_entry *depend) { struct nhg_connected *new = NULL; new = nhg_connected_new(depend); - if (new) - nhg_connected_tree_add(head, new); + /* On success, NULL will be returned from the + * RB code. + */ + if (new && (nhg_connected_tree_add(head, new) == NULL)) + return NULL; + + /* If it wasn't successful, it must be a duplicate. We enforce the + * unique property for the `nhg_connected` tree. + */ + nhg_connected_free(new); + + return depend; } static void @@ -1104,8 +1133,14 @@ done: static void depends_add(struct nhg_connected_tree_head *head, struct nhg_hash_entry *depend) { - nhg_connected_tree_add_nhe(head, depend); - zebra_nhg_increment_ref(depend); + /* If NULL is returned, it was successfully added and + * needs to have its refcnt incremented. + * + * Else the NHE is already present in the tree and doesn't + * need to increment the refcnt. + */ + if (nhg_connected_tree_add_nhe(head, depend) == NULL) + zebra_nhg_increment_ref(depend); } static struct nhg_hash_entry * diff --git a/zebra/zebra_nhg_private.h b/zebra/zebra_nhg_private.h index 79107b047c..92f438fcec 100644 --- a/zebra/zebra_nhg_private.h +++ b/zebra/zebra_nhg_private.h @@ -52,9 +52,22 @@ extern bool nhg_connected_tree_is_empty(const struct nhg_connected_tree_head *head); extern struct nhg_connected * nhg_connected_tree_root(struct nhg_connected_tree_head *head); -extern void nhg_connected_tree_del_nhe(struct nhg_connected_tree_head *head, - struct nhg_hash_entry *nhe); -extern void nhg_connected_tree_add_nhe(struct nhg_connected_tree_head *head, - struct nhg_hash_entry *nhe); + +/* I realize _add/_del returns are backwords. + * + * Currently the list APIs are not standardized for what happens in + * the _del() function when the item isn't present. + * + * We are choosing to return NULL if not found in the _del case for now. + */ + +/* Delete NHE from the tree. On success, return the NHE, otherwise NULL. */ +extern struct nhg_hash_entry * +nhg_connected_tree_del_nhe(struct nhg_connected_tree_head *head, + struct nhg_hash_entry *nhe); +/* ADD NHE to the tree. On success, return NULL, otherwise return the NHE. */ +extern struct nhg_hash_entry * +nhg_connected_tree_add_nhe(struct nhg_connected_tree_head *head, + struct nhg_hash_entry *nhe); #endif /* __ZEBRA_NHG_PRIVATE_H__ */ From c8b891b48392e0d29eb7b56942e4df697d4c14fd Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Mon, 6 Jan 2020 13:33:45 -0500 Subject: [PATCH 2/3] zebra: reset nexthop pointer in zread of nexthops We were not resetting the nexthop pointer to NULL for each new read of a nexthop from the zapi route. On the chance we get a nexthop that does not have a proper type, we will not create a new nexthop and update that pointer, thus it still has the last valid one and will create a group with two pointers to the same nexthop. Then when it enters any code that iterates the group, it loops endlessly. This was found with zapi fuzzing. ``` 0x00007f728891f1c3 in jhash2 (k=, length=, initval=12183506) at lib/jhash.c:138 0x00007f728896d92c in nexthop_hash (nexthop=) at lib/nexthop.c:563 0x00007f7288979ece in nexthop_group_hash (nhg=) at lib/nexthop_group.c:394 0x0000000000621036 in zebra_nhg_hash_key (arg=) at zebra/zebra_nhg.c:356 0x00007f72888ec0e1 in hash_get (hash=, data=0x7ffffb94aef0, alloc_func=0x0) at lib/hash.c:138 0x00007f72888ee118 in hash_lookup (hash=0x7f7288de2f10, data=0x7f728908e7fc) at lib/hash.c:183 0x0000000000626613 in zebra_nhg_find (nhe=0x7ffffb94b080, id=0, nhg=0x6020000032d0, nhg_depends=0x0, vrf_id=, afi=, type=) at zebra/zebra_nhg.c:541 0x0000000000625f39 in zebra_nhg_rib_find (id=0, nhg=, rt_afi=AFI_IP) at zebra/zebra_nhg.c:1126 0x000000000065f953 in rib_add_multipath (afi=AFI_IP, safi=, p=0x7ffffb94b370, src_p=0x0, re=0x6070000013d0, ng=0x7f728908e7fc) at zebra/zebra_rib.c:2616 0x0000000000768f90 in zread_route_add (client=0x61f000000080, hdr=, msg=, zvrf=) at zebra/zapi_msg.c:1596 0x000000000077c135 in zserv_handle_commands (client=, msg=0x61b000000780) at zebra/zapi_msg.c:2636 0x0000000000575e1f in main (argc=, argv=) at zebra/main.c:309 ``` ``` (gdb) p *nhg->nexthop $4 = {next = 0x5488e0, prev = 0x5488e0, vrf_id = 16843009, ifindex = 16843009, type = NEXTHOP_TYPE_IFINDEX, flags = 8 '\b', {gate = {ipv4 = {s_addr = 0}, ipv6 = {__in6_u = {__u6_addr8 = '\000' , __u6_addr16 = {0, 0, 0, 0, 0, 0, 0, 0}, __u6_addr32 = {0, 0, 0, 0}}}}, bh_type = BLACKHOLE_UNSPEC}, src = {ipv4 = {s_addr = 0}, ipv6 = {__in6_u = {__u6_addr8 = '\000' , __u6_addr16 = {0, 0, 0, 0, 0, 0, 0, 0}, __u6_addr32 = {0, 0, 0, 0}}}}, rmap_src = {ipv4 = {s_addr = 0}, ipv6 = {__in6_u = {__u6_addr8 = '\000' , __u6_addr16 = {0, 0, 0, 0, 0, 0, 0, 0}, __u6_addr32 = {0, 0, 0, 0}}}}, resolved = 0x0, rparent = 0x0, nh_label_type = ZEBRA_LSP_NONE, nh_label = 0x0, weight = 1 '\001'} (gdb) quit ``` Signed-off-by: Stephen Worley --- zebra/zapi_msg.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index c21d00bbe6..4fa7a3c164 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -1475,6 +1475,8 @@ static void zread_route_add(ZAPI_HANDLER_ARGS) api_nh = &api.nexthops[i]; ifindex_t ifindex = 0; + nexthop = NULL; + if (IS_ZEBRA_DEBUG_RECV) zlog_debug("nh type %d", api_nh->type); From a7e1b02d4a6794a5caa475ab6e4bfc30fdc6b31b Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Tue, 21 Jan 2020 15:03:52 -0500 Subject: [PATCH 3/3] zebra: add null check before connecting recursive depend Add a null check in `handle_recursive_depend()` so it doesn't try to add a NULL pointer to the RB tree. This was found with clang SA. Signed-off-by: Stephen Worley --- zebra/zebra_nhg.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 0925705a25..a3b414975e 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -510,7 +510,9 @@ static void handle_recursive_depend(struct nhg_connected_tree_head *nhg_depends, resolved_ng.nexthop = nh; depend = zebra_nhg_rib_find(0, &resolved_ng, afi); - depends_add(nhg_depends, depend); + + if (depend) + depends_add(nhg_depends, depend); } static bool zebra_nhg_find(struct nhg_hash_entry **nhe, uint32_t id,