From 9d3de36d0d09f76f5caf3caae18293a0b105b6fd Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sat, 18 Jun 2022 14:54:52 -0400 Subject: [PATCH 1/6] isisd: Let's use an actual NULL pointer to test for a NULL pointer Signed-off-by: Donald Sharp --- isisd/isis_bfd.c | 2 +- isisd/isis_snmp.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/isisd/isis_bfd.c b/isisd/isis_bfd.c index 5311a384e7..9e8267b6e8 100644 --- a/isisd/isis_bfd.c +++ b/isisd/isis_bfd.c @@ -206,7 +206,7 @@ static int bfd_handle_circuit_add_addr(struct isis_circuit *circuit) struct isis_adjacency *adj; struct listnode *node; - if (circuit->area == 0) + if (circuit->area == NULL) return 0; for (ALL_LIST_ELEMENTS_RO(circuit->area->adjacency_list, node, adj)) { diff --git a/isisd/isis_snmp.c b/isisd/isis_snmp.c index 3590044f85..a184b5b30a 100644 --- a/isisd/isis_snmp.c +++ b/isisd/isis_snmp.c @@ -2117,7 +2117,7 @@ static uint8_t *isis_snmp_find_circ(struct variable *v, oid *name, switch (v->magic) { case ISIS_CIRC_IFINDEX: - if (circuit->interface == 0) + if (circuit->interface == NULL) return SNMP_INTEGER(0); return SNMP_INTEGER(circuit->interface->ifindex); From 8b5153aab09695f19eed74b141919487369b58c2 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 22 Jun 2022 07:48:51 -0400 Subject: [PATCH 2/6] bgpd: Cleanup pointer assignment so compiler doesn't get confused Coverity SA thinks that the `struct prefix`.u.prefix4 is limited to actually 4 bytes of memory at that spot, but it's in a union and it can be treated as a prefix6 as well. Just change the pointer assignment to something that covers both easily. Signed-off-by: Donald Sharp --- bgpd/rfapi/vnc_zebra.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/bgpd/rfapi/vnc_zebra.c b/bgpd/rfapi/vnc_zebra.c index 672a0e9780..293f88d1df 100644 --- a/bgpd/rfapi/vnc_zebra.c +++ b/bgpd/rfapi/vnc_zebra.c @@ -628,7 +628,6 @@ static void vnc_zebra_add_del_nve(struct bgp *bgp, struct rfapi_descriptor *rfd, struct rfapi_nve_group_cfg *rfg = rfd->rfg; afi_t afi = family2afi(rfd->vn_addr.addr_family); struct prefix nhp; - // struct prefix *nhpp; void *pAddr; vnc_zlog_debug_verbose("%s: entry, add=%d", __func__, add); @@ -660,7 +659,7 @@ static void vnc_zebra_add_del_nve(struct bgp *bgp, struct rfapi_descriptor *rfd, return; } - pAddr = &nhp.u.prefix4; + pAddr = &nhp.u.val; /* * Loop over the list of NVE-Groups configured for From 75700af6027073a2ede58b429ec49b6beb05dcb3 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 22 Jun 2022 08:12:04 -0400 Subject: [PATCH 3/6] pimd: Limit pim's ecmp to what zebra tells us is the multipath Zebra can be setup to use a value that is less than MULTIPATH_NUM. When pimd connects to zebra, zebra will inform pim about the MULTIPATH_NUM used. Let's use that value for figuring out our multipath value. Signed-off-by: Donald Sharp --- pimd/pim_instance.h | 1 + pimd/pim_nht.c | 25 +++++++++++++------------ pimd/pim_rpf.c | 9 +++++---- pimd/pim_zebra.c | 1 + pimd/pim_zlookup.c | 2 +- pimd/pimd.c | 1 + 6 files changed, 22 insertions(+), 17 deletions(-) diff --git a/pimd/pim_instance.h b/pimd/pim_instance.h index b19e8208ba..23b9df4aff 100644 --- a/pimd/pim_instance.h +++ b/pimd/pim_instance.h @@ -88,6 +88,7 @@ struct pim_router { uint32_t register_suppress_time; int packet_process; uint32_t register_probe_time; + uint16_t multipath; /* * What is the default vrf that we work in diff --git a/pimd/pim_nht.c b/pimd/pim_nht.c index 22716c2a92..564d16a60b 100644 --- a/pimd/pim_nht.c +++ b/pimd/pim_nht.c @@ -304,15 +304,15 @@ bool pim_nht_bsr_rpf_check(struct pim_instance *pim, struct in_addr bsr_addr, * "check cache or get immediate result." But until that can * be worked in, here's a copy of the code below :( */ - struct pim_zlookup_nexthop nexthop_tab[MULTIPATH_NUM]; + struct pim_zlookup_nexthop nexthop_tab[router->multipath]; ifindex_t i; struct interface *ifp = NULL; int num_ifindex; memset(nexthop_tab, 0, sizeof(nexthop_tab)); - num_ifindex = zclient_lookup_nexthop(pim, nexthop_tab, - MULTIPATH_NUM, bsr_addr, - PIM_NEXTHOP_LOOKUP_MAX); + num_ifindex = zclient_lookup_nexthop( + pim, nexthop_tab, router->multipath, bsr_addr, + PIM_NEXTHOP_LOOKUP_MAX); if (num_ifindex <= 0) return false; @@ -501,8 +501,8 @@ static int pim_ecmp_nexthop_search(struct pim_instance *pim, struct prefix *src, struct prefix *grp, int neighbor_needed) { - struct pim_neighbor *nbrs[MULTIPATH_NUM], *nbr = NULL; - struct interface *ifps[MULTIPATH_NUM]; + struct pim_neighbor *nbrs[router->multipath], *nbr = NULL; + struct interface *ifps[router->multipath]; struct nexthop *nh_node = NULL; ifindex_t first_ifindex; struct interface *ifp = NULL; @@ -888,11 +888,11 @@ int pim_ecmp_nexthop_lookup(struct pim_instance *pim, struct prefix *grp, int neighbor_needed) { struct pim_nexthop_cache *pnc; - struct pim_zlookup_nexthop nexthop_tab[MULTIPATH_NUM]; - struct pim_neighbor *nbrs[MULTIPATH_NUM], *nbr = NULL; + struct pim_zlookup_nexthop nexthop_tab[router->multipath]; + struct pim_neighbor *nbrs[router->multipath], *nbr = NULL; struct pim_rpf rpf; int num_ifindex; - struct interface *ifps[MULTIPATH_NUM], *ifp; + struct interface *ifps[router->multipath], *ifp; int first_ifindex; int found = 0; uint8_t i = 0; @@ -915,9 +915,10 @@ int pim_ecmp_nexthop_lookup(struct pim_instance *pim, } memset(nexthop_tab, 0, - sizeof(struct pim_zlookup_nexthop) * MULTIPATH_NUM); - num_ifindex = zclient_lookup_nexthop(pim, nexthop_tab, MULTIPATH_NUM, - src_addr, PIM_NEXTHOP_LOOKUP_MAX); + sizeof(struct pim_zlookup_nexthop) * router->multipath); + num_ifindex = + zclient_lookup_nexthop(pim, nexthop_tab, router->multipath, + src_addr, PIM_NEXTHOP_LOOKUP_MAX); if (num_ifindex < 1) { if (PIM_DEBUG_PIM_NHT) zlog_warn( diff --git a/pimd/pim_rpf.c b/pimd/pim_rpf.c index c470bcd4d7..bd4dd31a2c 100644 --- a/pimd/pim_rpf.c +++ b/pimd/pim_rpf.c @@ -54,7 +54,7 @@ void pim_rpf_set_refresh_time(struct pim_instance *pim) bool pim_nexthop_lookup(struct pim_instance *pim, struct pim_nexthop *nexthop, pim_addr addr, int neighbor_needed) { - struct pim_zlookup_nexthop nexthop_tab[MULTIPATH_NUM]; + struct pim_zlookup_nexthop nexthop_tab[router->multipath]; struct pim_neighbor *nbr = NULL; int num_ifindex; struct interface *ifp = NULL; @@ -92,9 +92,10 @@ bool pim_nexthop_lookup(struct pim_instance *pim, struct pim_nexthop *nexthop, } memset(nexthop_tab, 0, - sizeof(struct pim_zlookup_nexthop) * MULTIPATH_NUM); - num_ifindex = zclient_lookup_nexthop(pim, nexthop_tab, MULTIPATH_NUM, - addr, PIM_NEXTHOP_LOOKUP_MAX); + sizeof(struct pim_zlookup_nexthop) * router->multipath); + num_ifindex = + zclient_lookup_nexthop(pim, nexthop_tab, router->multipath, + addr, PIM_NEXTHOP_LOOKUP_MAX); if (num_ifindex < 1) { zlog_warn( "%s %s: could not find nexthop ifindex for address %pPAs", diff --git a/pimd/pim_zebra.c b/pimd/pim_zebra.c index 4135ae0408..7f217d9c2e 100644 --- a/pimd/pim_zebra.c +++ b/pimd/pim_zebra.c @@ -452,6 +452,7 @@ static void pim_zebra_connected(struct zclient *zclient) static void pim_zebra_capabilities(struct zclient_capabilities *cap) { router->mlag_role = cap->role; + router->multipath = cap->ecmp; } static zclient_handler *const pim_handlers[] = { diff --git a/pimd/pim_zlookup.c b/pimd/pim_zlookup.c index a3978bd0d2..28777e4947 100644 --- a/pimd/pim_zlookup.c +++ b/pimd/pim_zlookup.c @@ -211,7 +211,7 @@ static int zclient_read_nexthop(struct pim_instance *pim, metric = stream_getl(s); nexthop_num = stream_getc(s); - if (nexthop_num < 1) { + if (nexthop_num < 1 || nexthop_num > router->multipath) { if (PIM_DEBUG_PIM_NHT_DETAIL) zlog_debug("%s: socket %d bad nexthop_num=%d", __func__, zlookup->sock, nexthop_num); diff --git a/pimd/pimd.c b/pimd/pimd.c index 58e874fd11..a3be3f66e1 100644 --- a/pimd/pimd.c +++ b/pimd/pimd.c @@ -100,6 +100,7 @@ void pim_router_init(void) router->debugs = 0; router->master = frr_init(); router->t_periodic = PIM_DEFAULT_T_PERIODIC; + router->multipath = MULTIPATH_NUM; /* RFC 4601: 4.6.3. Assert Metrics From d9529c9fb11d7fabe6f6986761293358dc0baffe Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 22 Jun 2022 08:24:03 -0400 Subject: [PATCH 4/6] ospf6d: Ensure that ospf6d does not memcpy beyond end of data Ensure that received data size can fit into temp variable that is used to dump data. Signed-off-by: Donald Sharp --- ospf6d/ospf6_auth_trailer.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/ospf6d/ospf6_auth_trailer.c b/ospf6d/ospf6_auth_trailer.c index 77ac4a1877..e54f6784e8 100644 --- a/ospf6d/ospf6_auth_trailer.c +++ b/ospf6d/ospf6_auth_trailer.c @@ -120,7 +120,13 @@ void ospf6_auth_hdr_dump_recv(struct ospf6_header *ospfh, uint16_t length, ospf6_at_hdr = (struct ospf6_auth_hdr *)((uint8_t *)ospfh + oh_len); at_hdr_len = ntohs(ospf6_at_hdr->length); - hash_len = at_hdr_len - OSPF6_AUTH_HDR_MIN_SIZE; + hash_len = at_hdr_len - (uint16_t)OSPF6_AUTH_HDR_MIN_SIZE; + if (hash_len > KEYCHAIN_MAX_HASH_SIZE) { + zlog_debug( + "Specified value for hash_len %u is greater than expected %u", + hash_len, KEYCHAIN_MAX_HASH_SIZE); + return; + } memcpy(temp, ospf6_at_hdr->data, hash_len); temp[hash_len] = '\0'; zlog_debug("OSPF6 Authentication Trailer"); From 6f55b4ae2405e548f687526b013b5ddcc4f6ff89 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 22 Jun 2022 09:57:08 -0400 Subject: [PATCH 5/6] pimd: Let end operator know the ifindex as well in failure case Signed-off-by: Donald Sharp --- pimd/pim_zlookup.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pimd/pim_zlookup.c b/pimd/pim_zlookup.c index 28777e4947..87cf434133 100644 --- a/pimd/pim_zlookup.c +++ b/pimd/pim_zlookup.c @@ -258,8 +258,9 @@ static int zclient_read_nexthop(struct pim_instance *pim, nexthop_tab[num_ifindex].ifindex = nh_ifi; ++num_ifindex; #else - zlog_warn("cannot use IPv4 nexthop %pI4 for IPv6 %pPA", - &nh_ip4, &addr); + zlog_warn( + "cannot use IPv4 nexthop %pI4(%d) for IPv6 %pPA", + &nh_ip4, nh_ifi, &addr); #endif break; case NEXTHOP_TYPE_IPV6_IFINDEX: From f4e8f5d496e54a55d0366b8fd7557f5bd65a0f2e Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 22 Jun 2022 19:40:58 -0400 Subject: [PATCH 6/6] pimd: Checks imply that pim is not properly configured The call to gm_update_ll checks for null pointers and implies to SA that things could not be configured correctly This is not true with the code flow. Remove the confusing code. Signed-off-by: Donald Sharp --- pimd/pim6_mld.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pimd/pim6_mld.c b/pimd/pim6_mld.c index a67d33ca97..255fd62ba7 100644 --- a/pimd/pim6_mld.c +++ b/pimd/pim6_mld.c @@ -2198,7 +2198,7 @@ void gm_ifp_teardown(struct interface *ifp) static void gm_update_ll(struct interface *ifp) { struct pim_interface *pim_ifp = ifp->info; - struct gm_if *gm_ifp = pim_ifp ? pim_ifp->mld : NULL; + struct gm_if *gm_ifp = pim_ifp->mld; bool was_querier; was_querier =