From f0507f05ce57398e135651896dace4cb68eeed54 Mon Sep 17 00:00:00 2001 From: Maria Matejka Date: Mon, 27 Sep 2021 16:40:28 +0200 Subject: Route sources have an explicit owner This commit prevents use-after-free of routes belonging to protocols which have been already destroyed, delaying also all the protocols' shutdown until all of their routes have been finally propagated through all the pipes down to the appropriate exports. The use-after-free was somehow hypothetic yet theoretically possible in rare conditions, when one BGP protocol authors a lot of routes and the user deletes that protocol by reconfiguring in the same time as next hop update is requested, causing rte_better() to be called on a not-yet-pruned network prefix while the owner protocol has been already freed. In parallel execution environments, this would happen an inter-thread use-after-free, causing possible heisenbugs or other nasty problems. --- proto/bgp/attrs.c | 20 +++++++++----------- proto/bgp/bgp.c | 15 ++++++++++----- proto/bgp/bgp.h | 7 +++++++ proto/bgp/packets.c | 5 +++++ 4 files changed, 31 insertions(+), 16 deletions(-) (limited to 'proto/bgp') diff --git a/proto/bgp/attrs.c b/proto/bgp/attrs.c index 892b26e3..9b9013f9 100644 --- a/proto/bgp/attrs.c +++ b/proto/bgp/attrs.c @@ -1670,9 +1670,8 @@ bgp_free_prefix(struct bgp_channel *c, struct bgp_prefix *px) int bgp_preexport(struct channel *c, rte *e) { - struct proto *SRC = e->src->proto; struct bgp_proto *p = (struct bgp_proto *) (c->proto); - struct bgp_proto *src = (SRC->proto == &proto_bgp) ? (struct bgp_proto *) SRC : NULL; + struct bgp_proto *src = bgp_rte_proto(e); /* Reject our routes */ if (src == p) @@ -1725,8 +1724,7 @@ bgp_preexport(struct channel *c, rte *e) static ea_list * bgp_update_attrs(struct bgp_proto *p, struct bgp_channel *c, rte *e, ea_list *attrs0, struct linpool *pool) { - struct proto *SRC = e->src->proto; - struct bgp_proto *src = (SRC->proto == &proto_bgp) ? (void *) SRC : NULL; + struct bgp_proto *src = bgp_rte_proto(e); struct bgp_export_state s = { .proto = p, .channel = c, .pool = pool, .src = src, .route = e, .mpls = c->desc->mpls }; ea_list *attrs = attrs0; eattr *a; @@ -1880,7 +1878,7 @@ bgp_get_neighbor(rte *r) return as; /* If AS_PATH is not defined, we treat rte as locally originated */ - struct bgp_proto *p = (void *) r->src->proto; + struct bgp_proto *p = bgp_rte_proto(r); return p->cf->confederation ?: p->local_as; } @@ -1910,8 +1908,8 @@ rte_stale(rte *r) int bgp_rte_better(rte *new, rte *old) { - struct bgp_proto *new_bgp = (struct bgp_proto *) new->src->proto; - struct bgp_proto *old_bgp = (struct bgp_proto *) old->src->proto; + struct bgp_proto *new_bgp = bgp_rte_proto(new); + struct bgp_proto *old_bgp = bgp_rte_proto(old); eattr *x, *y; u32 n, o; @@ -2055,8 +2053,8 @@ bgp_rte_better(rte *new, rte *old) int bgp_rte_mergable(rte *pri, rte *sec) { - struct bgp_proto *pri_bgp = (struct bgp_proto *) pri->src->proto; - struct bgp_proto *sec_bgp = (struct bgp_proto *) sec->src->proto; + struct bgp_proto *pri_bgp = bgp_rte_proto(pri); + struct bgp_proto *sec_bgp = bgp_rte_proto(sec); eattr *x, *y; u32 p, s; @@ -2137,8 +2135,8 @@ same_group(rte *r, u32 lpref, u32 lasn) static inline int use_deterministic_med(struct rte_storage *r) { - struct proto *P = r->rte.src->proto; - return (P->proto == &proto_bgp) && ((struct bgp_proto *) P)->cf->deterministic_med; + struct bgp_proto *p = bgp_rte_proto(&r->rte); + return p && p->cf->deterministic_med; } int diff --git a/proto/bgp/bgp.c b/proto/bgp/bgp.c index 5f2e7dfd..dc845550 100644 --- a/proto/bgp/bgp.c +++ b/proto/bgp/bgp.c @@ -1726,6 +1726,13 @@ done: return p->p.proto_state; } +struct rte_owner_class bgp_rte_owner_class = { + .get_route_info = bgp_get_route_info, + .rte_better = bgp_rte_better, + .rte_mergable = bgp_rte_mergable, + .rte_igp_metric = bgp_rte_igp_metric, +}; + static struct proto * bgp_init(struct proto_config *CF) { @@ -1739,10 +1746,9 @@ bgp_init(struct proto_config *CF) P->reload_routes = bgp_reload_routes; P->feed_begin = bgp_feed_begin; P->feed_end = bgp_feed_end; - P->rte_better = bgp_rte_better; - P->rte_mergable = bgp_rte_mergable; - P->rte_recalculate = cf->deterministic_med ? bgp_rte_recalculate : NULL; - P->rte_igp_metric = bgp_rte_igp_metric; + + P->sources.class = &bgp_rte_owner_class; + P->sources.rte_recalculate = cf->deterministic_med ? bgp_rte_recalculate : NULL; p->cf = cf; p->is_internal = (cf->local_as == cf->remote_as); @@ -2605,6 +2611,5 @@ struct protocol proto_bgp = { .copy_config = bgp_copy_config, .get_status = bgp_get_status, .get_attr = bgp_get_attr, - .get_route_info = bgp_get_route_info, .show_proto_info = bgp_show_proto_info }; diff --git a/proto/bgp/bgp.h b/proto/bgp/bgp.h index 342dc023..7cb4df1f 100644 --- a/proto/bgp/bgp.h +++ b/proto/bgp/bgp.h @@ -523,6 +523,7 @@ rta_resolvable(rta *a) return a->dest == RTD_UNICAST; } +extern struct rte_owner_class bgp_rte_owner_class; #ifdef LOCAL_DEBUG #define BGP_FORCE_DEBUG 1 @@ -594,6 +595,12 @@ int bgp_get_attr(const struct eattr *e, byte *buf, int buflen); void bgp_get_route_info(struct rte *, byte *); int bgp_total_aigp_metric_(rta *a, u64 *metric, const struct adata **ad); +static inline struct bgp_proto *bgp_rte_proto(struct rte *rte) +{ + return (rte->src->owner->class == &bgp_rte_owner_class) ? + SKIP_BACK(struct bgp_proto, p.sources, rte->src->owner) : NULL; +} + #define BGP_AIGP_METRIC 1 #define BGP_AIGP_MAX U64(0xffffffffffffffff) diff --git a/proto/bgp/packets.c b/proto/bgp/packets.c index 647551e5..d2d5b174 100644 --- a/proto/bgp/packets.c +++ b/proto/bgp/packets.c @@ -1339,6 +1339,8 @@ bgp_rte_update(struct bgp_parse_state *s, net_addr *n, u32 path_id, rta *a0) { if (path_id != s->last_id) { + rt_unlock_source(s->last_src); + s->last_src = rt_get_source(&s->proto->p, path_id); s->last_id = path_id; @@ -2421,6 +2423,7 @@ bgp_decode_nlri(struct bgp_parse_state *s, u32 afi, byte *nlri, uint len, ea_lis s->last_id = 0; s->last_src = s->proto->p.main_source; + rt_lock_source(s->last_src); /* * IPv4 BGP and MP-BGP may be used together in one update, therefore we do not @@ -2451,6 +2454,8 @@ bgp_decode_nlri(struct bgp_parse_state *s, u32 afi, byte *nlri, uint len, ea_lis rta_free(s->cached_rta); s->cached_rta = NULL; + + rt_unlock_source(s->last_src); } static void -- cgit v1.2.3