From 604816d5031a89e682ebaed1478c78501b922b6b Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Fri, 14 Jul 2023 12:14:20 -0400 Subject: [PATCH] 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 (cherry picked from commit 2ba2c284bab39f220eea3beabd5feeea216e3bfb) --- bgpd/bgpd.c | 107 ++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 83 insertions(+), 24 deletions(-) diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 6faaf8aae6..10a73f3e7c 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -5501,9 +5501,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, @@ -5516,16 +5521,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. @@ -5536,7 +5546,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; } @@ -5568,11 +5578,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; @@ -5606,13 +5621,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; } @@ -5635,6 +5655,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)) @@ -5643,10 +5667,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; @@ -7185,6 +7212,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; @@ -7198,9 +7226,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); @@ -7222,6 +7251,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)) @@ -7229,9 +7259,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); @@ -7264,11 +7296,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; } @@ -7288,6 +7325,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)) @@ -7295,9 +7336,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; @@ -7342,6 +7385,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)) @@ -7349,9 +7396,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); @@ -7381,11 +7430,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; } @@ -7404,6 +7457,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)) @@ -7411,9 +7468,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;