proxmox-mirrors/mirror_frr
1
0
Fork 0
mirror of https://git.proxmox.com/git/mirror_frr synced 2025-08-13 22:57:45 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

bgpd: Prevent use after free

Browse Source 
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 2ba2c284ba)
This commit is contained in:
Donald Sharp 2023-07-14 12:14:20 -04:00 committed by Mergify
parent 906452c90e
commit 604816d503
1 changed files with 83 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

107
bgpd/bgpd.c
View File

@ -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;