bgpd: modify attr fields before hash insert

bgp_attr_intern(attr) takes an attribute, duplicates it, and inserts it
into the attribute hash table, returning the inserted attr. This is done
when processing a bgp update. We store the returned attribute in the
path info struct. However, later on we modify one of the fields of the
attribute. This field is inspected by attrhash_cmp, the function that
allows the hash table to select the correct item from the hash chain for
a given key when doing a lookup on an item. By modifying the field after
it's been inserted, we open the possibility that two items in the same
chain that at insertion time were differential by attrhash_cmp becomes
equal according to that function. When performing subsequent hash
lookups, it is then indeterminate which of the equivalent items the hash
table will select from the chain (in practice it is the first one but
this may not be the one we want). Thus, it is illegal to modify
data used by a hash comparison function after inserting that data into
a hash table.

In fact this is occurring for attributes. We insert two attributes that
hash to the same key and thus end up in the same hash chain. Then we
modify one of them such that the two items now compare equal. Later one
we want to release the second item from the chain before XFREE()'ing it,
but since the two items compare equal we get the first item back, then
free the second one, which constitutes two bugs, the first being the
wrong attribute removed from the hash table and the second being a
dangling pointer stored in the hash table.

To rectify this we need to perform any modifications to an attr before
it is inserted into the table, i.e., before calling bgp_attr_intern().
This patch does that by moving the sole modification to the attr that
occurs after the insert (that I have seen) before that call.

Signed-off-by: Quentin Young <qlyoung@nvidia.com>

diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c
index 91acbf6c6d6d7e2985052c71d13c862a9f8cb8a6..8eaee36c2ee255481de0440fdb79ee2f38ad46f7 100644 (file)
--- a/bgpd/bgp_route.c
+++ b/bgpd/bgp_route.c
@@ -3605,6 +3605,12 @@ int bgp_update(struct peer *peer, const struct prefix *p, uint32_t addpath_id,
                goto filtered;
        }
 
+       /* Update Overlay Index */
+       if (afi == AFI_L2VPN) {
+               overlay_index_update(&new_attr,
+                                    evpn == NULL ? NULL : &evpn->gw_ip);
+       }
+
        attr_new = bgp_attr_intern(&new_attr);
 
        /* If maximum prefix count is configured and current prefix
@@ -3850,12 +3856,6 @@ int bgp_update(struct peer *peer, const struct prefix *p, uint32_t addpath_id,
                        }
                }
 #endif
-               /* Update Overlay Index */
-               if (afi == AFI_L2VPN) {
-                       overlay_index_update(
-                               pi->attr,
-                               evpn == NULL ? NULL : &evpn->gw_ip);
-               }
 
                /* Update bgp route dampening information.  */
                if (CHECK_FLAG(bgp->af_flags[afi][safi], BGP_CONFIG_DAMPENING)