From b53c5f1ab47d05a85b254e88f12be4ac5c71d42a Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Tue, 24 Sep 2019 19:34:39 -0400 Subject: [PATCH 1/4] isisd: circuit is derefed in every code path No need to check for circuit being null, we have already de-refed it in every code path and would have crashed before this point if it was. Signed-off-by: Donald Sharp --- isisd/isis_te.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/isisd/isis_te.c b/isisd/isis_te.c index 44fa45d02b..9871d2bcb9 100644 --- a/isisd/isis_te.c +++ b/isisd/isis_te.c @@ -81,8 +81,7 @@ void isis_link_params_update(struct isis_circuit *circuit, return; /* Sanity Check */ - if ((circuit == NULL) || (ifp == NULL) - || (circuit->state != C_STATE_UP)) + if ((ifp == NULL) || (circuit->state != C_STATE_UP)) return; zlog_debug("TE(%s): Update circuit parameters for interface %s", From 0f9f74baeb97f437d7acf7feda0f400d50943c4c Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Tue, 24 Sep 2019 20:40:08 -0400 Subject: [PATCH 2/4] ospf6d: Prevent use after free the for (ALL_LSDB...) macro was iterating over lsa, when lsa had just been freed in these functions. Remove the macro and make the adjustments saving lsa_next before the free. Signed-off-by: Donald Sharp --- ospf6d/ospf6_lsdb.c | 15 ++++++++++---- ospf6d/ospf6_message.c | 44 ++++++++++++++++++++++++++++------------- ospf6d/ospf6_neighbor.c | 31 +++++++++++++++++++++-------- 3 files changed, 64 insertions(+), 26 deletions(-) diff --git a/ospf6d/ospf6_lsdb.c b/ospf6d/ospf6_lsdb.c index b551dbdfa6..0a9f1c6f7c 100644 --- a/ospf6d/ospf6_lsdb.c +++ b/ospf6d/ospf6_lsdb.c @@ -298,13 +298,17 @@ struct ospf6_lsa *ospf6_lsdb_next(const struct route_node *iterend, void ospf6_lsdb_remove_all(struct ospf6_lsdb *lsdb) { - struct ospf6_lsa *lsa; + struct ospf6_lsa *lsa, *lsa_next; + const struct route_node *iterend; if (lsdb == NULL) return; - for (ALL_LSDB(lsdb, lsa)) + for (iterend = ospf6_lsdb_head(lsdb, 0, 0, 0, &lsa); lsa; + lsa = lsa_next) { + lsa_next = ospf6_lsdb_next(iterend, lsa); ospf6_lsdb_remove(lsa, lsdb); + } } void ospf6_lsdb_lsa_unlock(struct ospf6_lsa *lsa) @@ -319,9 +323,12 @@ void ospf6_lsdb_lsa_unlock(struct ospf6_lsa *lsa) int ospf6_lsdb_maxage_remover(struct ospf6_lsdb *lsdb) { int reschedule = 0; - struct ospf6_lsa *lsa; + struct ospf6_lsa *lsa, *lsa_next; + const struct route_node *iterend; - for (ALL_LSDB(lsdb, lsa)) { + for (iterend = ospf6_lsdb_head(lsdb, 0, 0, 0, &lsa); lsa; + lsa = lsa_next) { + lsa_next = ospf6_lsdb_next(iterend, lsa); if (!OSPF6_LSA_IS_MAXAGE(lsa)) continue; if (lsa->retrans_count != 0) { diff --git a/ospf6d/ospf6_message.c b/ospf6d/ospf6_message.c index 4acb5e3b2e..da42a24252 100644 --- a/ospf6d/ospf6_message.c +++ b/ospf6d/ospf6_message.c @@ -1866,7 +1866,8 @@ int ospf6_dbdesc_send(struct thread *thread) int ospf6_dbdesc_send_newone(struct thread *thread) { struct ospf6_neighbor *on; - struct ospf6_lsa *lsa; + struct ospf6_lsa *lsa, *lsa_next; + const struct route_node *iterend; unsigned int size = 0; on = (struct ospf6_neighbor *)THREAD_ARG(thread); @@ -1876,7 +1877,10 @@ int ospf6_dbdesc_send_newone(struct thread *thread) structure) so that ospf6_send_dbdesc () can send those LSAs */ size = sizeof(struct ospf6_lsa_header) + sizeof(struct ospf6_dbdesc); - for (ALL_LSDB(on->summary_list, lsa)) { + + for (iterend = ospf6_lsdb_head(on->summary_list, 0, 0, 0, &lsa); lsa; + lsa = lsa_next) { + lsa_next = ospf6_lsdb_next(iterend, lsa); if (size + sizeof(struct ospf6_lsa_header) > ospf6_packet_max(on->ospf6_if)) { ospf6_lsdb_lsa_unlock(lsa); @@ -2019,7 +2023,8 @@ int ospf6_lsupdate_send_neighbor(struct thread *thread) struct ospf6_lsupdate *lsupdate; uint8_t *p; int lsa_cnt; - struct ospf6_lsa *lsa; + struct ospf6_lsa *lsa, *lsa_next; + const struct route_node *iterend; on = (struct ospf6_neighbor *)THREAD_ARG(thread); on->thread_send_lsupdate = (struct thread *)NULL; @@ -2044,7 +2049,9 @@ int ospf6_lsupdate_send_neighbor(struct thread *thread) /* lsupdate_list lists those LSA which doesn't need to be retransmitted. remove those from the list */ - for (ALL_LSDB(on->lsupdate_list, lsa)) { + for (iterend = ospf6_lsdb_head(on->lsupdate_list, 0, 0, 0, &lsa); lsa; + lsa = lsa_next) { + lsa_next = ospf6_lsdb_next(iterend, lsa); /* MTU check */ if ((p - sendbuf + (unsigned int)OSPF6_LSA_SIZE(lsa->header)) > ospf6_packet_max(on->ospf6_if)) { @@ -2074,7 +2081,7 @@ int ospf6_lsupdate_send_neighbor(struct thread *thread) p += OSPF6_LSA_SIZE(lsa->header); lsa_cnt++; - assert(lsa->lock == 2); + assert(lsa->lock == 1); ospf6_lsdb_remove(lsa, on->lsupdate_list); } @@ -2202,7 +2209,8 @@ int ospf6_lsupdate_send_interface(struct thread *thread) struct ospf6_lsupdate *lsupdate; uint8_t *p; int lsa_cnt; - struct ospf6_lsa *lsa; + struct ospf6_lsa *lsa, *lsa_next; + const struct route_node *iterend; oi = (struct ospf6_interface *)THREAD_ARG(thread); oi->thread_send_lsupdate = (struct thread *)NULL; @@ -2228,7 +2236,9 @@ int ospf6_lsupdate_send_interface(struct thread *thread) p = (uint8_t *)((caddr_t)lsupdate + sizeof(struct ospf6_lsupdate)); lsa_cnt = 0; - for (ALL_LSDB(oi->lsupdate_list, lsa)) { + for (iterend = ospf6_lsdb_head(oi->lsupdate_list, 0, 0, 0, &lsa); lsa; + lsa = lsa_next) { + lsa_next = ospf6_lsdb_next(iterend, lsa); /* MTU check */ if ((p - sendbuf + ((unsigned int)OSPF6_LSA_SIZE(lsa->header))) > ospf6_packet_max(oi)) { @@ -2263,7 +2273,7 @@ int ospf6_lsupdate_send_interface(struct thread *thread) p += OSPF6_LSA_SIZE(lsa->header); lsa_cnt++; - assert(lsa->lock == 2); + assert(lsa->lock == 1); ospf6_lsdb_remove(lsa, oi->lsupdate_list); } @@ -2289,7 +2299,8 @@ int ospf6_lsack_send_neighbor(struct thread *thread) struct ospf6_neighbor *on; struct ospf6_header *oh; uint8_t *p; - struct ospf6_lsa *lsa; + struct ospf6_lsa *lsa, *lsa_next; + const struct route_node *iterend; int lsa_cnt = 0; on = (struct ospf6_neighbor *)THREAD_ARG(thread); @@ -2312,7 +2323,9 @@ int ospf6_lsack_send_neighbor(struct thread *thread) p = (uint8_t *)((caddr_t)oh + sizeof(struct ospf6_header)); - for (ALL_LSDB(on->lsack_list, lsa)) { + for (iterend = ospf6_lsdb_head(on->lsack_list, 0, 0, 0, &lsa); lsa; + lsa = lsa_next) { + lsa_next = ospf6_lsdb_next(iterend, lsa); /* MTU check */ if (p - sendbuf + sizeof(struct ospf6_lsa_header) > ospf6_packet_max(on->ospf6_if)) { @@ -2340,7 +2353,7 @@ int ospf6_lsack_send_neighbor(struct thread *thread) memcpy(p, lsa->header, sizeof(struct ospf6_lsa_header)); p += sizeof(struct ospf6_lsa_header); - assert(lsa->lock == 2); + assert(lsa->lock == 1); ospf6_lsdb_remove(lsa, on->lsack_list); lsa_cnt++; } @@ -2367,7 +2380,8 @@ int ospf6_lsack_send_interface(struct thread *thread) struct ospf6_interface *oi; struct ospf6_header *oh; uint8_t *p; - struct ospf6_lsa *lsa; + struct ospf6_lsa *lsa, *lsa_next; + const struct route_node *iterend; int lsa_cnt = 0; oi = (struct ospf6_interface *)THREAD_ARG(thread); @@ -2391,7 +2405,9 @@ int ospf6_lsack_send_interface(struct thread *thread) p = (uint8_t *)((caddr_t)oh + sizeof(struct ospf6_header)); - for (ALL_LSDB(oi->lsack_list, lsa)) { + for (iterend = ospf6_lsdb_head(oi->lsack_list, 0, 0, 0, &lsa); lsa; + lsa = lsa_next) { + lsa_next = ospf6_lsdb_next(iterend, lsa); /* MTU check */ if (p - sendbuf + sizeof(struct ospf6_lsa_header) > ospf6_packet_max(oi)) { @@ -2409,7 +2425,7 @@ int ospf6_lsack_send_interface(struct thread *thread) memcpy(p, lsa->header, sizeof(struct ospf6_lsa_header)); p += sizeof(struct ospf6_lsa_header); - assert(lsa->lock == 2); + assert(lsa->lock == 1); ospf6_lsdb_remove(lsa, oi->lsack_list); lsa_cnt++; } diff --git a/ospf6d/ospf6_neighbor.c b/ospf6d/ospf6_neighbor.c index dccf15aee2..4318db5225 100644 --- a/ospf6d/ospf6_neighbor.c +++ b/ospf6d/ospf6_neighbor.c @@ -112,11 +112,15 @@ struct ospf6_neighbor *ospf6_neighbor_create(uint32_t router_id, void ospf6_neighbor_delete(struct ospf6_neighbor *on) { - struct ospf6_lsa *lsa; + struct ospf6_lsa *lsa, *lsa_next; + const struct route_node *iterend; ospf6_lsdb_remove_all(on->summary_list); ospf6_lsdb_remove_all(on->request_list); - for (ALL_LSDB(on->retrans_list, lsa)) { + + for (iterend = ospf6_lsdb_head(on->retrans_list, 0, 0, 0, &lsa); lsa; + lsa = lsa_next) { + lsa_next = ospf6_lsdb_next(iterend, lsa); ospf6_decrement_retrans_count(lsa); ospf6_lsdb_remove(lsa, on->retrans_list); } @@ -287,7 +291,8 @@ int twoway_received(struct thread *thread) int negotiation_done(struct thread *thread) { struct ospf6_neighbor *on; - struct ospf6_lsa *lsa; + struct ospf6_lsa *lsa, *lsa_next; + const struct route_node *iterend; on = (struct ospf6_neighbor *)THREAD_ARG(thread); assert(on); @@ -301,7 +306,10 @@ int negotiation_done(struct thread *thread) /* clear ls-list */ ospf6_lsdb_remove_all(on->summary_list); ospf6_lsdb_remove_all(on->request_list); - for (ALL_LSDB(on->retrans_list, lsa)) { + + for (iterend = ospf6_lsdb_head(on->retrans_list, 0, 0, 0, &lsa); lsa; + lsa = lsa_next) { + lsa_next = ospf6_lsdb_next(iterend, lsa); ospf6_decrement_retrans_count(lsa); ospf6_lsdb_remove(lsa, on->retrans_list); } @@ -495,7 +503,8 @@ int seqnumber_mismatch(struct thread *thread) int bad_lsreq(struct thread *thread) { struct ospf6_neighbor *on; - struct ospf6_lsa *lsa; + struct ospf6_lsa *lsa, *lsa_next; + const struct route_node *iterend; on = (struct ospf6_neighbor *)THREAD_ARG(thread); assert(on); @@ -514,7 +523,10 @@ int bad_lsreq(struct thread *thread) ospf6_lsdb_remove_all(on->summary_list); ospf6_lsdb_remove_all(on->request_list); - for (ALL_LSDB(on->retrans_list, lsa)) { + + for (iterend = ospf6_lsdb_head(on->retrans_list, 0, 0, 0, &lsa); lsa; + lsa = lsa_next) { + lsa_next = ospf6_lsdb_next(iterend, lsa); ospf6_decrement_retrans_count(lsa); ospf6_lsdb_remove(lsa, on->retrans_list); } @@ -532,7 +544,8 @@ int bad_lsreq(struct thread *thread) int oneway_received(struct thread *thread) { struct ospf6_neighbor *on; - struct ospf6_lsa *lsa; + struct ospf6_lsa *lsa, *lsa_next; + const struct route_node *iterend; on = (struct ospf6_neighbor *)THREAD_ARG(thread); assert(on); @@ -549,7 +562,9 @@ int oneway_received(struct thread *thread) ospf6_lsdb_remove_all(on->summary_list); ospf6_lsdb_remove_all(on->request_list); - for (ALL_LSDB(on->retrans_list, lsa)) { + for (iterend = ospf6_lsdb_head(on->retrans_list, 0, 0, 0, &lsa); lsa; + lsa = lsa_next) { + lsa_next = ospf6_lsdb_next(iterend, lsa); ospf6_decrement_retrans_count(lsa); ospf6_lsdb_remove(lsa, on->retrans_list); } From 38513e880e789680c4a8590fcb1a99d260aa534f Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Tue, 24 Sep 2019 20:48:10 -0400 Subject: [PATCH 3/4] eigrpd: On creation of socket ensure vrf exists If the vrf does not exist, politely do not create the socket. Signed-off-by: Donald Sharp --- eigrpd/eigrp_network.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/eigrpd/eigrp_network.c b/eigrpd/eigrp_network.c index c7ffbf9f0e..3e09ec41bb 100644 --- a/eigrpd/eigrp_network.c +++ b/eigrpd/eigrp_network.c @@ -55,12 +55,15 @@ static void eigrp_network_run_interface(struct eigrp *, struct prefix *, int eigrp_sock_init(struct vrf *vrf) { - int eigrp_sock; + int eigrp_sock = -1; int ret; #ifdef IP_HDRINCL int hincl = 1; #endif + if (!vrf) + return eigrp_sock; + frr_with_privs(&eigrpd_privs) { eigrp_sock = vrf_socket( AF_INET, SOCK_RAW, IPPROTO_EIGRPIGP, vrf->vrf_id, From 24cbd1309881c9fdb0a925fac91339661cef41a2 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Tue, 24 Sep 2019 20:48:56 -0400 Subject: [PATCH 4/4] pimd: up->channel_oil cannot be NULL When we create the up data structure we create the channel_oil as well. As such it is impossible to get into this code so it can be removed. Signed-off-by: Donald Sharp --- pimd/pim_zebra.c | 55 ------------------------------------------------ 1 file changed, 55 deletions(-) diff --git a/pimd/pim_zebra.c b/pimd/pim_zebra.c index b0db23f54a..37cb12548c 100644 --- a/pimd/pim_zebra.c +++ b/pimd/pim_zebra.c @@ -1180,12 +1180,6 @@ void pim_forward_start(struct pim_ifchannel *ch) { struct pim_upstream *up = ch->upstream; uint32_t mask = PIM_OIF_FLAG_PROTO_PIM; - int input_iface_vif_index = 0; - struct pim_instance *pim; - struct pim_interface *pim_ifp; - - pim_ifp = ch->interface->info; - pim = pim_ifp->pim; if (PIM_DEBUG_PIM_TRACE) { char source_str[INET_ADDRSTRLEN]; @@ -1203,55 +1197,6 @@ void pim_forward_start(struct pim_ifchannel *ch) inet_ntoa(up->upstream_addr)); } - /* Resolve IIF for upstream as mroute_del sets mfcc_parent to MAXVIFS, - as part of mroute_del called by pim_forward_stop. - */ - if ((up->upstream_addr.s_addr != INADDR_ANY) && (!up->channel_oil)) { - struct prefix src, grp; - - grp.family = AF_INET; - grp.prefixlen = IPV4_MAX_BITLEN; - grp.u.prefix4 = up->sg.grp; - src.family = AF_INET; - src.prefixlen = IPV4_MAX_BITLEN; - src.u.prefix4 = up->sg.src; - - if (pim_ecmp_nexthop_lookup(pim, &up->rpf.source_nexthop, &src, - &grp, 0)) - input_iface_vif_index = pim_if_find_vifindex_by_ifindex( - pim, up->rpf.source_nexthop.interface->ifindex); - - if (input_iface_vif_index < 1) { - if (PIM_DEBUG_PIM_TRACE) { - char source_str[INET_ADDRSTRLEN]; - pim_inet4_dump("", up->sg.src, - source_str, sizeof(source_str)); - zlog_debug( - "%s %s: could not find input interface for source %s", - __FILE__, __PRETTY_FUNCTION__, - source_str); - } - pim_channel_oil_change_iif(pim, up->channel_oil, - MAXVIFS, - __PRETTY_FUNCTION__); - } - - else - pim_channel_oil_change_iif(pim, up->channel_oil, - input_iface_vif_index, - __PRETTY_FUNCTION__); - - if (PIM_DEBUG_TRACE) { - struct interface *in_intf = pim_if_find_by_vif_index( - pim, input_iface_vif_index); - zlog_debug( - "%s: Update channel_oil IIF %s VIFI %d entry %s ", - __PRETTY_FUNCTION__, - in_intf ? in_intf->name : "Unknown", - input_iface_vif_index, up->sg_str); - } - } - if (up->flags & PIM_UPSTREAM_FLAG_MASK_SRC_IGMP) mask = PIM_OIF_FLAG_PROTO_IGMP;