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 <sharpd@nvidia.com>

diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c
index 9811b364bf1d495eae7b33d2cb224d4b58078b41..b04ee2dd54401894cfc5d5d497f2ab1e334c9ebc 100644 (file)
--- 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);