bgpd: Prevent possible crash when parsing v6 attributes

The peer->nexthop.ifp pointer must be set when parsing the
attributes in bgp_mp_reach_parse, notice this
and fail gracefully.

Rework bgp_nexthop_set to remove the HAVE_CUMULUS and to
fail the nexthop_set when we have a zebra connection and
no ifp pointer, as that not havinga zebra connection and
no ifp pointer is legal.

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c
index 6acd4c8cf1670d4f692df891d61ab386377237aa..720afe6e098b945c13e9fe43cb288cf8de3a92ab 100644 (file)
--- a/bgpd/bgp_attr.c
+++ b/bgpd/bgp_attr.c
@@ -1707,8 +1707,14 @@ int bgp_mp_reach_parse(struct bgp_attr_parser_args *args,
                        stream_getl(s); /* RD low */
                }
                stream_get(&attr->mp_nexthop_global, s, IPV6_MAX_BYTELEN);
-               if (IN6_IS_ADDR_LINKLOCAL(&attr->mp_nexthop_global))
+               if (IN6_IS_ADDR_LINKLOCAL(&attr->mp_nexthop_global)) {
+                       if (!peer->nexthop.ifp) {
+                               zlog_warn("%s: interface not set appropriately to handle some attributes",
+                                         peer->host);
+                               return BGP_ATTR_PARSE_WITHDRAW;
+                       }
                        attr->nh_ifindex = peer->nexthop.ifp->ifindex;
+               }
                break;
        case BGP_ATTR_NHLEN_IPV6_GLOBAL_AND_LL:
        case BGP_ATTR_NHLEN_VPNV6_GLOBAL_AND_LL:
@@ -1718,8 +1724,14 @@ int bgp_mp_reach_parse(struct bgp_attr_parser_args *args,
                        stream_getl(s); /* RD low */
                }
                stream_get(&attr->mp_nexthop_global, s, IPV6_MAX_BYTELEN);
-               if (IN6_IS_ADDR_LINKLOCAL(&attr->mp_nexthop_global))
+               if (IN6_IS_ADDR_LINKLOCAL(&attr->mp_nexthop_global)) {
+                       if (!peer->nexthop.ifp) {
+                               zlog_warn("%s: interface not set appropriately to handle some attributes",
+                                         peer->host);
+                               return BGP_ATTR_PARSE_WITHDRAW;
+                       }
                        attr->nh_ifindex = peer->nexthop.ifp->ifindex;
+               }
                if (attr->mp_nexthop_len
                    == BGP_ATTR_NHLEN_VPNV6_GLOBAL_AND_LL) {
                        stream_getl(s); /* RD high */
@@ -1743,6 +1755,11 @@ int bgp_mp_reach_parse(struct bgp_attr_parser_args *args,
 
                        attr->mp_nexthop_len = IPV6_MAX_BYTELEN;
                }
+               if (!peer->nexthop.ifp) {
+                       zlog_warn("%s: Interface not set appropriately to handle this some attributes",
+                                 peer->host);
+                       return BGP_ATTR_PARSE_WITHDRAW;
+               }
                attr->nh_lla_ifindex = peer->nexthop.ifp->ifindex;
                break;
        default:

diff --git a/bgpd/bgp_network.c b/bgpd/bgp_network.c
index 22d5d35c822160389a59636dbeb1be4f373cf56f..f6cad646174a45dafc81d415b780068abff16983 100644 (file)
--- a/bgpd/bgp_network.c
+++ b/bgpd/bgp_network.c
@@ -36,6 +36,7 @@
 #include "filter.h"
 #include "ns.h"
 #include "lib_errors.h"
+#include "nexthop.h"
 
 #include "bgpd/bgpd.h"
 #include "bgpd/bgp_open.h"
@@ -44,6 +45,7 @@
 #include "bgpd/bgp_debug.h"
 #include "bgpd/bgp_errors.h"
 #include "bgpd/bgp_network.h"
+#include "bgpd/bgp_zebra.h"
 
 extern struct zebra_privs_t bgpd_privs;
 
@@ -617,15 +619,12 @@ int bgp_getsockname(struct peer *peer)
        if (!peer->su_remote)
                return -1;
 
-       if (bgp_nexthop_set(peer->su_local, peer->su_remote, &peer->nexthop,
-                           peer)) {
-#if defined(HAVE_CUMULUS)
-               flog_err(
-                       BGP_ERR_NH_UPD,
-                       "%s: nexthop_set failed, resetting connection - intf %p",
-                       peer->host, peer->nexthop.ifp);
+       if (!bgp_zebra_nexthop_set(peer->su_local, peer->su_remote,
+                                  &peer->nexthop, peer)) {
+               flog_err(BGP_ERR_NH_UPD,
+                        "%s: nexthop_set failed, resetting connection - intf %p",
+                        peer->host, peer->nexthop.ifp);
                return -1;
-#endif
        }
        return 0;
 }

diff --git a/bgpd/bgp_zebra.c b/bgpd/bgp_zebra.c
index 43afc317e9b30e9a1ed4b19d2d9deda01842ce64..49fed16121e5d4652bc8bc7dd76331bff4a03813 100644 (file)
--- a/bgpd/bgp_zebra.c
+++ b/bgpd/bgp_zebra.c
@@ -777,8 +777,9 @@ static int if_get_ipv4_address(struct interface *ifp, struct in_addr *addr)
        return 0;
 }
 
-int bgp_nexthop_set(union sockunion *local, union sockunion *remote,
-                   struct bgp_nexthop *nexthop, struct peer *peer)
+
+bool bgp_zebra_nexthop_set(union sockunion *local, union sockunion *remote,
+                          struct bgp_nexthop *nexthop, struct peer *peer)
 {
        int ret = 0;
        struct interface *ifp = NULL;
@@ -786,9 +787,9 @@ int bgp_nexthop_set(union sockunion *local, union sockunion *remote,
        memset(nexthop, 0, sizeof(struct bgp_nexthop));
 
        if (!local)
-               return -1;
+               return false;
        if (!remote)
-               return -1;
+               return false;
 
        if (local->sa.sa_family == AF_INET) {
                nexthop->v4 = local->sin.sin_addr;
@@ -815,8 +816,24 @@ int bgp_nexthop_set(union sockunion *local, union sockunion *remote,
                                                      peer->bgp->vrf_id);
        }
 
-       if (!ifp)
-               return -1;
+       if (!ifp) {
+               /*
+                * BGP views do not currently get proper data
+                * from zebra( when attached ) to be able to
+                * properly resolve nexthops, so give this
+                * instance type a pass.
+                */
+               if (peer->bgp->inst_type == BGP_INSTANCE_TYPE_VIEW)
+                       return true;
+               /*
+                * If we have no interface data but we have established
+                * some connection w/ zebra than something has gone
+                * terribly terribly wrong here, so say this failed
+                * If we do not any zebra connection then not
+                * having a ifp pointer is ok.
+                */
+               return zclient_num_connects ? false : true;
+       }
 
        nexthop->ifp = ifp;
 
@@ -912,7 +929,7 @@ int bgp_nexthop_set(union sockunion *local, union sockunion *remote,
 
        /* If we have identified the local interface, there is no error for now.
         */
-       return 0;
+       return true;
 }
 
 static struct in6_addr *bgp_info_to_ipv6_nexthop(struct bgp_info *info,

diff --git a/bgpd/bgp_zebra.h b/bgpd/bgp_zebra.h
index 546d72402af819ef288f9a4a0914cfae50e56cb7..c00d9925eeff24c3db05bc490a731a9ca7d385f3 100644 (file)
--- a/bgpd/bgp_zebra.h
+++ b/bgpd/bgp_zebra.h
@@ -73,6 +73,9 @@ extern int bgp_zebra_advertise_all_vni(struct bgp *, int);
 
 extern int bgp_zebra_num_connects(void);
 
+extern bool bgp_zebra_nexthop_set(union sockunion *, union sockunion *,
+                                 struct bgp_nexthop *, struct peer *);
+
 struct bgp_pbr_action;
 struct bgp_pbr_match;
 struct bgp_pbr_match_entry;

diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h
index 8a997413901c0cc0f46d03889a867ab44ac6553e..0a8962b4c703dde608356e45adf19d02f8bae2c3 100644 (file)
--- a/bgpd/bgpd.h
+++ b/bgpd/bgpd.h
@@ -1470,8 +1470,6 @@ extern void bgp_terminate(void);
 extern void bgp_reset(void);
 extern time_t bgp_clock(void);
 extern void bgp_zclient_reset(void);
-extern int bgp_nexthop_set(union sockunion *, union sockunion *,
-                          struct bgp_nexthop *, struct peer *);
 extern struct bgp *bgp_get_default(void);
 extern struct bgp *bgp_lookup(as_t, const char *);
 extern struct bgp *bgp_lookup_by_name(const char *);