bgpd: Prevent use after free

When running bgp_always_compare_med, I am frequently seeing a crash
After running with valgrind I am seeing this and a invalid write
immediately after this as well.

==311743== Invalid read of size 2
==311743==    at 0x4992421: route_map_counter_decrement (routemap.c:3308)
==311743==    by 0x35664D: peer_route_map_unset (bgpd.c:7259)
==311743==    by 0x306546: peer_route_map_unset_vty (bgp_vty.c:8037)
==311743==    by 0x3066AC: no_neighbor_route_map (bgp_vty.c:8081)
==311743==    by 0x49078DE: cmd_execute_command_real (command.c:990)
==311743==    by 0x4907A63: cmd_execute_command (command.c:1050)
==311743==    by 0x490801F: cmd_execute (command.c:1217)
==311743==    by 0x49C5535: vty_command (vty.c:551)
==311743==    by 0x49C7459: vty_execute (vty.c:1314)
==311743==    by 0x49C97D1: vtysh_read (vty.c:2223)
==311743==    by 0x49BE5E2: event_call (event.c:1995)
==311743==    by 0x494786C: frr_run (libfrr.c:1204)
==311743==    by 0x1F7655: main (bgp_main.c:505)
==311743==  Address 0x9ec2180 is 64 bytes inside a block of size 120 free'd
==311743==    at 0x484B27F: free (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==311743==    by 0x495A1BA: qfree (memory.c:130)
==311743==    by 0x498D412: route_map_free_map (routemap.c:748)
==311743==    by 0x498D176: route_map_add (routemap.c:672)
==311743==    by 0x498D79B: route_map_get (routemap.c:857)
==311743==    by 0x499C256: lib_route_map_create (routemap_northbound.c:102)
==311743==    by 0x49702D8: nb_callback_create (northbound.c:1234)
==311743==    by 0x497107F: nb_callback_configuration (northbound.c:1578)
==311743==    by 0x4971693: nb_transaction_process (northbound.c:1709)
==311743==    by 0x496FCF4: nb_candidate_commit_apply (northbound.c:1103)
==311743==    by 0x496FE4E: nb_candidate_commit (northbound.c:1136)
==311743==    by 0x497798F: nb_cli_classic_commit (northbound_cli.c:49)
==311743==    by 0x4977B4F: nb_cli_pending_commit_check (northbound_cli.c:88)
==311743==    by 0x49078C1: cmd_execute_command_real (command.c:987)
==311743==    by 0x4907B44: cmd_execute_command (command.c:1068)
==311743==    by 0x490801F: cmd_execute (command.c:1217)
==311743==    by 0x49C5535: vty_command (vty.c:551)
==311743==    by 0x49C7459: vty_execute (vty.c:1314)
==311743==    by 0x49C97D1: vtysh_read (vty.c:2223)
==311743==    by 0x49BE5E2: event_call (event.c:1995)
==311743==    by 0x494786C: frr_run (libfrr.c:1204)
==311743==    by 0x1F7655: main (bgp_main.c:505)
==311743==  Block was alloc'd at
==311743==    at 0x484DA83: calloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==311743==    by 0x495A068: qcalloc (memory.c:105)
==311743==    by 0x498D0C8: route_map_new (routemap.c:646)
==311743==    by 0x498D128: route_map_add (routemap.c:658)
==311743==    by 0x498D79B: route_map_get (routemap.c:857)
==311743==    by 0x499C256: lib_route_map_create (routemap_northbound.c:102)
==311743==    by 0x49702D8: nb_callback_create (northbound.c:1234)
==311743==    by 0x497107F: nb_callback_configuration (northbound.c:1578)
==311743==    by 0x4971693: nb_transaction_process (northbound.c:1709)
==311743==    by 0x496FCF4: nb_candidate_commit_apply (northbound.c:1103)
==311743==    by 0x496FE4E: nb_candidate_commit (northbound.c:1136)
==311743==    by 0x497798F: nb_cli_classic_commit (northbound_cli.c:49)
==311743==    by 0x4977B4F: nb_cli_pending_commit_check (northbound_cli.c:88)
==311743==    by 0x49078C1: cmd_execute_command_real (command.c:987)
==311743==    by 0x4907B44: cmd_execute_command (command.c:1068)
==311743==    by 0x490801F: cmd_execute (command.c:1217)
==311743==    by 0x49C5535: vty_command (vty.c:551)
==311743==    by 0x49C7459: vty_execute (vty.c:1314)
==311743==    by 0x49C97D1: vtysh_read (vty.c:2223)
==311743==    by 0x49BE5E2: event_call (event.c:1995)
==311743==    by 0x494786C: frr_run (libfrr.c:1204)

Effectively the route_map that is being stored has been freed already
but we have not cleaned up properly yet.  Go through and clean the
code up by ensuring that the pointer actually exists instead of trusting
it does when doing the decrement operation.

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

diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c
index 5c97f90c5525986a5d9b76d5b522e958ba1e51c4..00a5e190cbd8a3b38bf4f841157f838c791ce5d9 100644 (file)
--- a/bgpd/bgpd.c
+++ b/bgpd/bgpd.c
@@ -5506,9 +5506,14 @@ int peer_default_originate_set(struct peer *peer, afi_t afi, safi_t safi,
        if (rmap) {
                if (!peer->default_rmap[afi][safi].name
                    || strcmp(rmap, peer->default_rmap[afi][safi].name) != 0) {
-                       if (peer->default_rmap[afi][safi].name)
+                       struct route_map *map = NULL;
+
+                       if (peer->default_rmap[afi][safi].name) {
+                               map = route_map_lookup_by_name(
+                                       peer->default_rmap[afi][safi].name);
                                XFREE(MTYPE_ROUTE_MAP_NAME,
                                      peer->default_rmap[afi][safi].name);
+                       }
 
                        /*
                         * When there is a change in route-map policy,
@@ -5521,16 +5526,21 @@ int peer_default_originate_set(struct peer *peer, afi_t afi, safi_t safi,
                                UNSET_FLAG(subgrp->sflags,
                                           SUBGRP_STATUS_DEFAULT_ORIGINATE);
 
-                       route_map_counter_decrement(peer->default_rmap[afi][safi].map);
+                       route_map_counter_decrement(map);
                        peer->default_rmap[afi][safi].name =
                                XSTRDUP(MTYPE_ROUTE_MAP_NAME, rmap);
                        peer->default_rmap[afi][safi].map = route_map;
                        route_map_counter_increment(route_map);
                }
        } else if (!rmap) {
-               if (peer->default_rmap[afi][safi].name)
+               struct route_map *map = NULL;
+
+               if (peer->default_rmap[afi][safi].name) {
+                       map = route_map_lookup_by_name(
+                               peer->default_rmap[afi][safi].name);
                        XFREE(MTYPE_ROUTE_MAP_NAME,
                              peer->default_rmap[afi][safi].name);
+               }
 
                /*
                 * This is triggered in case of route-map deletion.
@@ -5541,7 +5551,7 @@ int peer_default_originate_set(struct peer *peer, afi_t afi, safi_t safi,
                        UNSET_FLAG(subgrp->sflags,
                                   SUBGRP_STATUS_DEFAULT_ORIGINATE);
 
-               route_map_counter_decrement(peer->default_rmap[afi][safi].map);
+               route_map_counter_decrement(map);
                peer->default_rmap[afi][safi].name = NULL;
                peer->default_rmap[afi][safi].map = NULL;
        }
@@ -5573,11 +5583,16 @@ int peer_default_originate_set(struct peer *peer, afi_t afi, safi_t safi,
                SET_FLAG(member->af_flags[afi][safi],
                         PEER_FLAG_DEFAULT_ORIGINATE);
                if (rmap) {
-                       if (member->default_rmap[afi][safi].name)
+                       struct route_map *map = NULL;
+
+                       if (member->default_rmap[afi][safi].name) {
+                               map = route_map_lookup_by_name(
+                                       member->default_rmap[afi][safi].name);
                                XFREE(MTYPE_ROUTE_MAP_NAME,
                                      member->default_rmap[afi][safi].name);
-                       route_map_counter_decrement(
-                                       member->default_rmap[afi][safi].map);
+                       }
+
+                       route_map_counter_decrement(map);
                        member->default_rmap[afi][safi].name =
                                XSTRDUP(MTYPE_ROUTE_MAP_NAME, rmap);
                        member->default_rmap[afi][safi].map = route_map;
@@ -5611,13 +5626,18 @@ int peer_default_originate_unset(struct peer *peer, afi_t afi, safi_t safi)
                PEER_ATTR_INHERIT(peer, peer->group,
                                  default_rmap[afi][safi].map);
        } else {
+               struct route_map *map = NULL;
+
                /* Otherwise remove flag and configuration from peer. */
                peer_af_flag_unset(peer, afi, safi,
                                   PEER_FLAG_DEFAULT_ORIGINATE);
-               if (peer->default_rmap[afi][safi].name)
+               if (peer->default_rmap[afi][safi].name) {
+                       map = route_map_lookup_by_name(
+                               peer->default_rmap[afi][safi].name);
                        XFREE(MTYPE_ROUTE_MAP_NAME,
                              peer->default_rmap[afi][safi].name);
-               route_map_counter_decrement(peer->default_rmap[afi][safi].map);
+               }
+               route_map_counter_decrement(map);
                peer->default_rmap[afi][safi].name = NULL;
                peer->default_rmap[afi][safi].map = NULL;
        }
@@ -5640,6 +5660,10 @@ int peer_default_originate_unset(struct peer *peer, afi_t afi, safi_t safi)
         * they are explicitly overriding peer-group configuration.
         */
        for (ALL_LIST_ELEMENTS(peer->group->peer, node, nnode, member)) {
+               struct route_map *map;
+
+               map = NULL;
+
                /* Skip peers with overridden configuration. */
                if (CHECK_FLAG(member->af_flags_override[afi][safi],
                               PEER_FLAG_DEFAULT_ORIGINATE))
@@ -5648,10 +5672,13 @@ int peer_default_originate_unset(struct peer *peer, afi_t afi, safi_t safi)
                /* Remove flag and configuration on peer-group member. */
                UNSET_FLAG(member->af_flags[afi][safi],
                           PEER_FLAG_DEFAULT_ORIGINATE);
-               if (member->default_rmap[afi][safi].name)
+               if (member->default_rmap[afi][safi].name) {
+                       map = route_map_lookup_by_name(
+                               member->default_rmap[afi][safi].name);
                        XFREE(MTYPE_ROUTE_MAP_NAME,
                              member->default_rmap[afi][safi].name);
-               route_map_counter_decrement(member->default_rmap[afi][safi].map);
+               }
+               route_map_counter_decrement(map);
                member->default_rmap[afi][safi].name = NULL;
                member->default_rmap[afi][safi].map = NULL;
 
@@ -7186,6 +7213,7 @@ int peer_route_map_set(struct peer *peer, afi_t afi, safi_t safi, int direct,
        struct peer *member;
        struct bgp_filter *filter;
        struct listnode *node, *nnode;
+       struct route_map *map = NULL;
 
        if (direct != RMAP_IN && direct != RMAP_OUT)
                return BGP_ERR_INVALID_VALUE;
@@ -7199,9 +7227,10 @@ int peer_route_map_set(struct peer *peer, afi_t afi, safi_t safi, int direct,
                if (strcmp(filter->map[direct].name, name) == 0)
                        return 0;
 
+               map = route_map_lookup_by_name(filter->map[direct].name);
                XFREE(MTYPE_BGP_FILTER_NAME, filter->map[direct].name);
        }
-       route_map_counter_decrement(filter->map[direct].map);
+       route_map_counter_decrement(map);
        filter->map[direct].name = XSTRDUP(MTYPE_BGP_FILTER_NAME, name);
        filter->map[direct].map = route_map;
        route_map_counter_increment(route_map);
@@ -7223,6 +7252,7 @@ int peer_route_map_set(struct peer *peer, afi_t afi, safi_t safi, int direct,
         * explicitly overriding peer-group configuration.
         */
        for (ALL_LIST_ELEMENTS(peer->group->peer, node, nnode, member)) {
+               map = NULL;
                /* Skip peers with overridden configuration. */
                if (CHECK_FLAG(member->filter_override[afi][safi][direct],
                               PEER_FT_ROUTE_MAP))
@@ -7230,9 +7260,11 @@ int peer_route_map_set(struct peer *peer, afi_t afi, safi_t safi, int direct,
 
                /* Set configuration on peer-group member. */
                filter = &member->filter[afi][safi];
-               if (filter->map[direct].name)
+               if (filter->map[direct].name) {
+                       map = route_map_lookup_by_name(filter->map[direct].name);
                        XFREE(MTYPE_BGP_FILTER_NAME, filter->map[direct].name);
-               route_map_counter_decrement(filter->map[direct].map);
+               }
+               route_map_counter_decrement(map);
                filter->map[direct].name = XSTRDUP(MTYPE_BGP_FILTER_NAME, name);
                filter->map[direct].map = route_map;
                route_map_counter_increment(route_map);
@@ -7265,11 +7297,16 @@ int peer_route_map_unset(struct peer *peer, afi_t afi, safi_t safi, int direct)
                PEER_ATTR_INHERIT(peer, peer->group,
                                  filter[afi][safi].map[direct].map);
        } else {
+               struct route_map *map = NULL;
+
                /* Otherwise remove configuration from peer. */
                filter = &peer->filter[afi][safi];
-               if (filter->map[direct].name)
+
+               if (filter->map[direct].name) {
+                       map = route_map_lookup_by_name(filter->map[direct].name);
                        XFREE(MTYPE_BGP_FILTER_NAME, filter->map[direct].name);
-               route_map_counter_decrement(filter->map[direct].map);
+               }
+               route_map_counter_decrement(map);
                filter->map[direct].name = NULL;
                filter->map[direct].map = NULL;
        }
@@ -7289,6 +7326,10 @@ int peer_route_map_unset(struct peer *peer, afi_t afi, safi_t safi, int direct)
         * explicitly overriding peer-group configuration.
         */
        for (ALL_LIST_ELEMENTS(peer->group->peer, node, nnode, member)) {
+               struct route_map *map;
+
+               map = NULL;
+
                /* Skip peers with overridden configuration. */
                if (CHECK_FLAG(member->filter_override[afi][safi][direct],
                               PEER_FT_ROUTE_MAP))
@@ -7296,9 +7337,11 @@ int peer_route_map_unset(struct peer *peer, afi_t afi, safi_t safi, int direct)
 
                /* Remove configuration on peer-group member. */
                filter = &member->filter[afi][safi];
-               if (filter->map[direct].name)
+               if (filter->map[direct].name) {
+                       map = route_map_lookup_by_name(filter->map[direct].name);
                        XFREE(MTYPE_BGP_FILTER_NAME, filter->map[direct].name);
-               route_map_counter_decrement(filter->map[direct].map);
+               }
+               route_map_counter_decrement(map);
                filter->map[direct].name = NULL;
                filter->map[direct].map = NULL;
 
@@ -7343,6 +7386,10 @@ int peer_unsuppress_map_set(struct peer *peer, afi_t afi, safi_t safi,
         * explicitly overriding peer-group configuration.
         */
        for (ALL_LIST_ELEMENTS(peer->group->peer, node, nnode, member)) {
+               struct route_map *map;
+
+               map = NULL;
+
                /* Skip peers with overridden configuration. */
                if (CHECK_FLAG(member->filter_override[afi][safi][0],
                               PEER_FT_UNSUPPRESS_MAP))
@@ -7350,9 +7397,11 @@ int peer_unsuppress_map_set(struct peer *peer, afi_t afi, safi_t safi,
 
                /* Set configuration on peer-group member. */
                filter = &member->filter[afi][safi];
-               if (filter->usmap.name)
+               if (filter->usmap.name) {
+                       map = route_map_lookup_by_name(filter->usmap.name);
                        XFREE(MTYPE_BGP_FILTER_NAME, filter->usmap.name);
-               route_map_counter_decrement(filter->usmap.map);
+               }
+               route_map_counter_decrement(map);
                filter->usmap.name = XSTRDUP(MTYPE_BGP_FILTER_NAME, name);
                filter->usmap.map = route_map;
                route_map_counter_increment(route_map);
@@ -7382,11 +7431,15 @@ int peer_unsuppress_map_unset(struct peer *peer, afi_t afi, safi_t safi)
                PEER_ATTR_INHERIT(peer, peer->group,
                                  filter[afi][safi].usmap.map);
        } else {
+               struct route_map *map = NULL;
+
                /* Otherwise remove configuration from peer. */
                filter = &peer->filter[afi][safi];
-               if (filter->usmap.name)
+               if (filter->usmap.name) {
+                       map = route_map_lookup_by_name(filter->usmap.name);
                        XFREE(MTYPE_BGP_FILTER_NAME, filter->usmap.name);
-               route_map_counter_decrement(filter->usmap.map);
+               }
+               route_map_counter_decrement(map);
                filter->usmap.name = NULL;
                filter->usmap.map = NULL;
        }
@@ -7405,6 +7458,10 @@ int peer_unsuppress_map_unset(struct peer *peer, afi_t afi, safi_t safi)
         * explicitly overriding peer-group configuration.
         */
        for (ALL_LIST_ELEMENTS(peer->group->peer, node, nnode, member)) {
+               struct route_map *map;
+
+               map = NULL;
+
                /* Skip peers with overridden configuration. */
                if (CHECK_FLAG(member->filter_override[afi][safi][0],
                               PEER_FT_UNSUPPRESS_MAP))
@@ -7412,9 +7469,11 @@ int peer_unsuppress_map_unset(struct peer *peer, afi_t afi, safi_t safi)
 
                /* Remove configuration on peer-group member. */
                filter = &member->filter[afi][safi];
-               if (filter->usmap.name)
+               if (filter->usmap.name) {
+                       map = route_map_lookup_by_name(filter->usmap.name);
                        XFREE(MTYPE_BGP_FILTER_NAME, filter->usmap.name);
-               route_map_counter_decrement(filter->usmap.map);
+               }
+               route_map_counter_decrement(map);
                filter->usmap.name = NULL;
                filter->usmap.map = NULL;