| From 992dc3de27747af7f11a344ff07f04767e24d85d Mon Sep 17 00:00:00 2001 | |
| From: Date Huang <[email protected]> | |
| Date: Thu, 20 Nov 2025 03:12:23 +0800 | |
| Subject: [PATCH] bgpd: Crash due to usage of freed up evpn_overlay attr | |
| Reason: | |
| Use-after-free in evpn_overlay due to incorrect reference counting when storing | |
| routes in Adj-RIB-In. | |
| 1) In bgp_update_receive, we parse the attributes first in bgp_attr_parse_ret | |
| and it will intern those attributes, so refcnt for them would become 0->1. We | |
| can't do evpn_overlay attribute processing here, as it requires NLRI/EVPN | |
| knowledge, which happens via bgp_nlri_parse later. So we don't know yet if | |
| evpn_overlay attribute would be created or not. | |
| 2) Later in bgp_update, the evpn_overlay is created with refcnt=0 and assigned to | |
| attr, stored in adj_in via bgp_adj_in_set(), then interned (refcnt→1). For all | |
| other used attributes, the refcnt goes to 1->2. | |
| 3) At the end of bgp_update_receive(), bgp_attr_unintern_sub(attr) is called, which | |
| causes evpn_overlay attribute to have refcnt 1->0, so it frees it up, leaving | |
| adj_in->attr->evpn_overlay with a dangling pointer. Any other attributes that are | |
| used have refcnt 2->1, so they are not freed up. | |
| 4) On next call to bgp_update, in bgp_adj_in_set(), when we try to lookup attr from | |
| adj_in->attr, it doesn't find a match as the old attr in adj_in->attr has freed | |
| up evpn_overlay. So in bgp_adj_in_set, we try to unintern the corrupted attr, | |
| causing hash lookup failure (the key has changed as attr->evpn_overlay is freed | |
| up, and that is part of hash generation) and assert. | |
| Fix: | |
| Intern evpn_overlay immediately before assigning to attr in the Adj-RIB-In path. | |
| This implements the double-refcount pattern used by other attributes like ecommunity: | |
| evpn_overlay_intern() sets refcnt=1, bgp_attr_intern() increments to refcnt=2, | |
| bgp_attr_unintern_sub() decrements to refcnt=1, leaving adj_in->attr with a | |
| valid reference. | |
| Signed-off-by: Soumya Roy <[email protected]> | |
| --- | |
| bgpd/bgp_attr.c | 2 +- | |
| bgpd/bgp_attr.h | 1 + | |
| bgpd/bgp_route.c | 4 +++- | |
| 3 files changed, 5 insertions(+), 2 deletions(-) | |
| diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c | |
| index 1a2fa8318e..4c552f7fc4 100644 | |
| --- a/bgpd/bgp_attr.c | |
| +++ b/bgpd/bgp_attr.c | |
| @@ -549,7 +549,7 @@ void evpn_overlay_free(struct bgp_route_evpn *bre) | |
| XFREE(MTYPE_BGP_EVPN_OVERLAY, bre); | |
| } | |
| -static struct bgp_route_evpn *evpn_overlay_intern(struct bgp_route_evpn *bre) | |
| +struct bgp_route_evpn *evpn_overlay_intern(struct bgp_route_evpn *bre) | |
| { | |
| struct bgp_route_evpn *find; | |
| diff --git a/bgpd/bgp_attr.h b/bgpd/bgp_attr.h | |
| index 087c3f35eb..6aa9c61eea 100644 | |
| --- a/bgpd/bgp_attr.h | |
| +++ b/bgpd/bgp_attr.h | |
| @@ -660,5 +660,6 @@ bgp_attr_set_vnc_subtlvs(struct attr *attr, | |
| extern bool route_matches_soo(struct bgp_path_info *pi, struct ecommunity *soo); | |
| extern void evpn_overlay_free(struct bgp_route_evpn *bre); | |
| +extern struct bgp_route_evpn *evpn_overlay_intern(struct bgp_route_evpn *bre); | |
| #endif /* _QUAGGA_BGP_ATTR_H */ | |
| diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c | |
| index 85cad88979..c8b4c7af09 100644 | |
| --- a/bgpd/bgp_route.c | |
| +++ b/bgpd/bgp_route.c | |
| @@ -4641,8 +4641,10 @@ void bgp_update(struct peer *peer, const struct prefix *p, uint32_t addpath_id, | |
| * attr->evpn_overlay, so that, this can be stored in adj_in. | |
| */ | |
| if (evpn) { | |
| - if (afi == AFI_L2VPN) | |
| + if (afi == AFI_L2VPN) { | |
| + evpn = evpn_overlay_intern(evpn); | |
| bgp_attr_set_evpn_overlay(attr, evpn); | |
| + } | |
| else | |
| evpn_overlay_free(evpn); | |
| } | |
| -- | |
| 2.51.0 | |
File Metadata
File Metadata
- Mime Type
- text/x-diff
- Storage Engine
- blob
- Storage Format
- Raw Data
- Storage Handle
- 3056802
- Default Alt Text
- 0001-bgpd-Crash-due-to-usage-of-freed-up-evpn_overlay-att.patch (3 KB)