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>
(cherry picked from commit 2ba2c284bab39f220eea3beabd5feeea216e3bfb)

diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c
index 494ad76507436fa0afcb4c7ae821a64c373fc04d..964349bba0c8f30b2bbbd98f0f099ea0c605c6a7 100644 (file)
--- a/bgpd/bgpd.c
+++ b/bgpd/bgpd.c
@@ -5421,9 +5421,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,
@@ -5436,16 +5441,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.
@@ -5456,7 +5466,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;
        }
@@ -5488,11 +5498,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;
@@ -5526,13 +5541,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;
        }
@@ -5555,6 +5575,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))
@@ -5563,10 +5587,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;
 
@@ -7094,6 +7121,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;
@@ -7107,9 +7135,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);
@@ -7131,6 +7160,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))
@@ -7138,9 +7168,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);
@@ -7173,11 +7205,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;
        }
@@ -7197,6 +7234,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))
@@ -7204,9 +7245,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;
 
@@ -7251,6 +7294,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))
@@ -7258,9 +7305,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);
@@ -7290,11 +7339,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;
        }
@@ -7313,6 +7366,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))
@@ -7320,9 +7377,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;