From 23882159fafd62e39d6cb5d569b0930134d61df3 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Mon, 28 Nov 2022 15:16:15 -0500 Subject: [PATCH] bgpd: Prevent use after free of peer structure When changing the peers sockunion structure the bgp->peer list was not being updated properly. Since the peer's su is being used for a sorted insert then the change of it requires that the value be pulled out of the bgp->peer list and then put back into as well. Additionally ensure that the hash is always released on peer deletion. Lead to this from this decode in a address sanitizer run. ================================================================= ==30778==ERROR: AddressSanitizer: heap-use-after-free on address 0x62a0000d8440 at pc 0x7f48c9c5c547 bp 0x7ffcba272cb0 sp 0x7ffcba272ca8 READ of size 2 at 0x62a0000d8440 thread T0 #0 0x7f48c9c5c546 in sockunion_same lib/sockunion.c:425 #1 0x55cfefe3000f in peer_hash_same bgpd/bgpd.c:890 #2 0x7f48c9bde039 in hash_release lib/hash.c:209 #3 0x55cfefe3373f in bgp_peer_conf_if_to_su_update bgpd/bgpd.c:1541 #4 0x55cfefd0be7a in bgp_stop bgpd/bgp_fsm.c:1631 #5 0x55cfefe4028f in peer_delete bgpd/bgpd.c:2362 #6 0x55cfefdd5e97 in no_neighbor_interface_config bgpd/bgp_vty.c:4267 #7 0x7f48c9b9d160 in cmd_execute_command_real lib/command.c:949 #8 0x7f48c9ba1112 in cmd_execute_command lib/command.c:1009 #9 0x7f48c9ba1573 in cmd_execute lib/command.c:1162 #10 0x7f48c9c87402 in vty_command lib/vty.c:526 #11 0x7f48c9c87832 in vty_execute lib/vty.c:1291 #12 0x7f48c9c8e741 in vtysh_read lib/vty.c:2130 #13 0x7f48c9c7a66d in thread_call lib/thread.c:1585 #14 0x7f48c9bf64e7 in frr_run lib/libfrr.c:1123 #15 0x55cfefc75a15 in main bgpd/bgp_main.c:540 #16 0x7f48c96b009a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a) #17 0x55cfefc787f9 in _start (/usr/lib/frr/bgpd+0xe27f9) 0x62a0000d8440 is located 576 bytes inside of 23376-byte region [0x62a0000d8200,0x62a0000ddd50) freed by thread T0 here: #0 0x7f48c9eb9fb0 in __interceptor_free (/lib/x86_64-linux-gnu/libasan.so.5+0xe8fb0) #1 0x55cfefe3fe42 in peer_free bgpd/bgpd.c:1113 #2 0x55cfefe3fe42 in peer_unlock_with_caller bgpd/bgpd.c:1144 #3 0x55cfefe4092e in peer_delete bgpd/bgpd.c:2457 #4 0x55cfefdd5e97 in no_neighbor_interface_config bgpd/bgp_vty.c:4267 #5 0x7f48c9b9d160 in cmd_execute_command_real lib/command.c:949 #6 0x7f48c9ba1112 in cmd_execute_command lib/command.c:1009 #7 0x7f48c9ba1573 in cmd_execute lib/command.c:1162 #8 0x7f48c9c87402 in vty_command lib/vty.c:526 #9 0x7f48c9c87832 in vty_execute lib/vty.c:1291 #10 0x7f48c9c8e741 in vtysh_read lib/vty.c:2130 #11 0x7f48c9c7a66d in thread_call lib/thread.c:1585 #12 0x7f48c9bf64e7 in frr_run lib/libfrr.c:1123 #13 0x55cfefc75a15 in main bgpd/bgp_main.c:540 #14 0x7f48c96b009a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a) Signed-off-by: Donald Sharp --- bgpd/bgpd.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 61 insertions(+), 9 deletions(-) diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 9811b364bf1d..b04ee2dd5440 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -1613,15 +1613,18 @@ void bgp_peer_conf_if_to_su_update(struct peer *peer) struct interface *ifp; int prev_family; int peer_addr_updated = 0; + struct listnode *node; + union sockunion old_su; + /* + * This function is only ever needed when FRR an interface + * based peering, so this simple test will tell us if + * we are in an interface based configuration or not + */ if (!peer->conf_if) return; - /* - * Our peer structure is stored in the bgp->peerhash - * release it before we modify anything. - */ - hash_release(peer->bgp->peerhash, peer); + old_su = peer->su; prev_family = peer->su.sa.sa_family; if ((ifp = if_lookup_by_name(peer->conf_if, peer->bgp->vrf_id))) { @@ -1664,9 +1667,48 @@ void bgp_peer_conf_if_to_su_update(struct peer *peer) } /* - * Since our su changed we need to del/add peer to the peerhash + * If they are the same, nothing to do here, move along */ - (void)hash_get(peer->bgp->peerhash, peer, hash_alloc_intern); + if (!sockunion_same(&old_su, &peer->su)) { + union sockunion new_su = peer->su; + struct bgp *bgp = peer->bgp; + + /* + * Our peer structure is stored in the bgp->peerhash + * release it before we modify anything in both the + * hash and the list. But *only* if the peer + * is in the bgp->peerhash as that on deletion + * we call bgp_stop which calls this function :( + * so on deletion let's remove from the list first + * and then do the deletion preventing this from + * being added back on the list below when we + * fail to remove it up here. + */ + + /* + * listnode_lookup just scans the list + * for the peer structure so it's safe + * to use without modifying the su + */ + node = listnode_lookup(bgp->peer, peer); + if (node) { + /* + * Let's reset the peer->su release and + * reset it and put it back. We have to + * do this because hash_release will + * scan through looking for a matching + * su if needed. + */ + peer->su = old_su; + hash_release(peer->bgp->peerhash, peer); + listnode_delete(peer->bgp->peer, peer); + + peer->su = new_su; + (void)hash_get(peer->bgp->peerhash, peer, + hash_alloc_intern); + listnode_add_sort(peer->bgp->peer, peer); + } + } } void bgp_recalculate_afi_safi_bestpaths(struct bgp *bgp, afi_t afi, safi_t safi) @@ -1816,6 +1858,7 @@ struct peer *peer_create_accept(struct bgp *bgp) peer = peer_lock(peer); /* bgp peer list reference */ listnode_add_sort(bgp->peer, peer); + (void)hash_get(bgp->peerhash, peer, hash_alloc_intern); return peer; } @@ -2509,9 +2552,16 @@ int peer_delete(struct peer *peer) /* Delete from all peer list. */ if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP) && (pn = listnode_lookup(bgp->peer, peer))) { - peer_unlock(peer); /* bgp peer list reference */ + /* + * Removing from the list node first because + * peer_unlock *can* call peer_delete( I know, + * I know ). So let's remove it and in + * the su recalculate function we'll ensure + * it's in there or not. + */ list_delete_node(bgp->peer, pn); hash_release(bgp->peerhash, peer); + peer_unlock(peer); /* bgp peer list reference */ } /* Buffers. */ @@ -3727,8 +3777,10 @@ int bgp_delete(struct bgp *bgp) for (ALL_LIST_ELEMENTS(bgp->group, node, next, group)) peer_group_delete(group); - for (ALL_LIST_ELEMENTS(bgp->peer, node, next, peer)) + while (listcount(bgp->peer)) { + peer = listnode_head(bgp->peer); peer_delete(peer); + } if (bgp->peer_self) { peer_delete(bgp->peer_self);