Skip to content

Commit

Permalink
zebra: fix table heap-after-free crash
Browse files Browse the repository at this point in the history 
Fix a heap-after-free that causes zebra to crash even without
address-sanitizer. To reproduce:

> echo "100 my_table" | tee -a /etc/iproute2/rt_tables
> ip route add blackhole default table 100
> ip route show table 100
> ip l add red type vrf table 100
> ip l del red
> ip route del blackhole default table 100

Zebra manages routing tables for all existing Linux RT tables,
regardless of whether they are assigned to a VRF interface. When a table
is not assigned to any VRF, zebra arbitrarily assigns it to the default
VRF, even though this is not strictly accurate (the code expects this
behavior).

When an RT table is created after a VRF, zebra correctly assigns the
table to the VRF. However, if a VRF interface is assigned to an existing
RT table, zebra does not update the table owner, which remains as the
default VRF. As a result, existing routing entries remain under the
default VRF, while new entries are correctly assigned to the VRF. The
VRF mismatch is unexpected in the code and creates crashes and memory
related issues.

Furthermore, Linux does not automatically delete RT tables when they are
unassigned from a VRF. It is incorrect to delete these tables from zebra.

Instead, at VRF disabling, do not release the table but reassign it to
the default VRF. At VRF enabling, change the table owner back to the
appropriate VRF.

> ==2866266==ERROR: AddressSanitizer: heap-use-after-free on address 0x606000154f54 at pc 0x7fa32474b83f bp 0x7ffe94f67d90 sp 0x7ffe94f67d88
> READ of size 1 at 0x606000154f54 thread T0
>     #0 0x7fa32474b83e in rn_hash_node_const_find lib/table.c:28
>     #1 0x7fa32474bab1 in rn_hash_node_find lib/table.c:28
>     #2 0x7fa32474d783 in route_node_get lib/table.c:283
>     #3 0x7fa3247328dd in srcdest_rnode_get lib/srcdest_table.c:231
>     FRRouting#4 0x55b0e4fa8da4 in rib_find_rn_from_ctx zebra/zebra_rib.c:1957
>     FRRouting#5 0x55b0e4fa8e31 in rib_process_result zebra/zebra_rib.c:1988
>     FRRouting#6 0x55b0e4fb9d64 in rib_process_dplane_results zebra/zebra_rib.c:4894
>     FRRouting#7 0x7fa32476689c in event_call lib/event.c:1996
>     FRRouting#8 0x7fa32463b7b2 in frr_run lib/libfrr.c:1232
>     FRRouting#9 0x55b0e4e6c32a in main zebra/main.c:526
>     FRRouting#10 0x7fa32424fd09 in __libc_start_main ../csu/libc-start.c:308
>     FRRouting#11 0x55b0e4e2d649 in _start (/usr/lib/frr/zebra+0x1a1649)
>
> 0x606000154f54 is located 20 bytes inside of 56-byte region [0x606000154f40,0x606000154f78)
> freed by thread T0 here:
>     #0 0x7fa324ca9b6f in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:123
>     #1 0x7fa324668d8f in qfree lib/memory.c:130
>     #2 0x7fa32474c421 in route_table_free lib/table.c:126
>     #3 0x7fa32474bf96 in route_table_finish lib/table.c:46
>     FRRouting#4 0x55b0e4fbca3a in zebra_router_free_table zebra/zebra_router.c:191
>     FRRouting#5 0x55b0e4fbccea in zebra_router_release_table zebra/zebra_router.c:214
>     FRRouting#6 0x55b0e4fd428e in zebra_vrf_disable zebra/zebra_vrf.c:219
>     FRRouting#7 0x7fa32476fabf in vrf_disable lib/vrf.c:326
>     FRRouting#8 0x7fa32476f5d4 in vrf_delete lib/vrf.c:231
>     FRRouting#9 0x55b0e4e4ad36 in interface_vrf_change zebra/interface.c:1478
>     FRRouting#10 0x55b0e4e4d5d2 in zebra_if_dplane_ifp_handling zebra/interface.c:1949
>     FRRouting#11 0x55b0e4e4fb89 in zebra_if_dplane_result zebra/interface.c:2268
>     FRRouting#12 0x55b0e4fb9f26 in rib_process_dplane_results zebra/zebra_rib.c:4954
>     FRRouting#13 0x7fa32476689c in event_call lib/event.c:1996
>     FRRouting#14 0x7fa32463b7b2 in frr_run lib/libfrr.c:1232
>     FRRouting#15 0x55b0e4e6c32a in main zebra/main.c:526
>     FRRouting#16 0x7fa32424fd09 in __libc_start_main ../csu/libc-start.c:308
>
> previously allocated by thread T0 here:
>     #0 0x7fa324caa037 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:154
>     #1 0x7fa324668c4d in qcalloc lib/memory.c:105
>     #2 0x7fa32474bf33 in route_table_init_with_delegate lib/table.c:38
>     #3 0x7fa32474e73c in route_table_init lib/table.c:512
>     FRRouting#4 0x55b0e4fbc353 in zebra_router_get_table zebra/zebra_router.c:137
>     FRRouting#5 0x55b0e4fd4da0 in zebra_vrf_table_create zebra/zebra_vrf.c:358
>     FRRouting#6 0x55b0e4fd3d30 in zebra_vrf_enable zebra/zebra_vrf.c:140
>     FRRouting#7 0x7fa32476f9b2 in vrf_enable lib/vrf.c:286
>     FRRouting#8 0x55b0e4e4af76 in interface_vrf_change zebra/interface.c:1533
>     FRRouting#9 0x55b0e4e4d612 in zebra_if_dplane_ifp_handling zebra/interface.c:1968
>     FRRouting#10 0x55b0e4e4fb89 in zebra_if_dplane_result zebra/interface.c:2268
>     FRRouting#11 0x55b0e4fb9f26 in rib_process_dplane_results zebra/zebra_rib.c:4954
>     FRRouting#12 0x7fa32476689c in event_call lib/event.c:1996
>     FRRouting#13 0x7fa32463b7b2 in frr_run lib/libfrr.c:1232
>     FRRouting#14 0x55b0e4e6c32a in main zebra/main.c:526
>     FRRouting#15 0x7fa32424fd09 in __libc_start_main ../csu/libc-start.c:308

Fixes: d8612e6 ("zebra: Track tables allocated by vrf and cleanup")
Signed-off-by: Louis Scalbert <[email protected]>
  • Loading branch information
@louis-6wind
louis-6wind committed Oct 16, 2024
1 parent cc63dbb commit abd5dc7
Show file tree
Hide file tree
Showing 3 changed files with 142 additions and 3 deletions.
63 changes: 63 additions & 0 deletions zebra/zebra_nhg.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2923,6 +2923,69 @@ static uint32_t proto_nhg_nexthop_active_update(struct nexthop_group *nhg)
return curr_active;
}

void nexthop_vrf_update(struct route_node *rn, struct route_entry *re,
vrf_id_t vrf_id)
{
struct nhg_hash_entry *curr_nhe, *new_nhe;
afi_t rt_afi = family2afi(rn->p.family);
struct nexthop *nexthop;

re->vrf_id = vrf_id;

/* Make a local copy of the existing nhe, so we don't work on/modify
* the shared nhe.
*/
curr_nhe = zebra_nhe_copy(re->nhe, re->nhe->id);

if (IS_ZEBRA_DEBUG_NHG_DETAIL)
zlog_debug("%s: re %p nhe %p (%pNG), curr_nhe %p", __func__, re,
re->nhe, re->nhe, curr_nhe);

/* Clear the existing id, if any: this will avoid any confusion
* if the id exists, and will also force the creation
* of a new nhe reflecting the changes we may make in this local copy.
*/
curr_nhe->id = 0;

curr_nhe->vrf_id = vrf_id;
for (ALL_NEXTHOPS(curr_nhe->nhg, nexthop)) {
if (!nexthop->ifindex)
/* change VRF ID of nexthop without interfaces
* (eg. blackhole)
*/
nexthop->vrf_id = vrf_id;
}

if (zebra_nhg_get_backup_nhg(curr_nhe)) {
for (ALL_NEXTHOPS(curr_nhe->backup_info->nhe->nhg, nexthop)) {
if (!nexthop->ifindex)
/* change VRF ID of nexthop without interfaces
* (eg. blackhole)
*/
nexthop->vrf_id = vrf_id;
}
}

/*
* Ref or create an nhe that matches the current state of the
* nexthop(s).
*/
new_nhe = zebra_nhg_rib_find_nhe(curr_nhe, rt_afi);

if (IS_ZEBRA_DEBUG_NHG_DETAIL)
zlog_debug("%s: re %p CHANGED: nhe %p (%pNG) => new_nhe %p (%pNG)",
__func__, re, re->nhe, re->nhe, new_nhe, new_nhe);

route_entry_update_nhe(re, new_nhe);

/*
* Do not need the old / copied nhe anymore since it
* was either copied over into a new nhe or not
* used at all.
*/
zebra_nhg_free(curr_nhe);
}

/*
* This function takes the start of two comparable nexthops from two different
* nexthop groups and walks them to see if they can be considered the same
Expand Down
2 changes: 2 additions & 0 deletions zebra/zebra_nhg.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,8 @@ extern void zebra_nhg_mark_keep(void);

/* Nexthop resolution processing */
struct route_entry; /* Forward ref to avoid circular includes */
extern void nexthop_vrf_update(struct route_node *rn, struct route_entry *re,
vrf_id_t vrf_id);
extern int nexthop_active_update(struct route_node *rn, struct route_entry *re,
struct route_entry *old_re);

Expand Down
80 changes: 77 additions & 3 deletions zebra/zebra_vrf.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,46 @@ static int zebra_vrf_enable(struct vrf *vrf)
return 0;
}

/* update the VRF ID of a routing table and their routing entries */
static void zebra_vrf_disable_update_vrfid(struct zebra_vrf *zvrf, afi_t afi,
safi_t safi)
{
struct rib_table_info *info;
struct route_entry *re;
struct route_node *rn;
bool empty_table = true;

/* Assign the table to the default VRF.
* Although the table is not technically owned by the default VRF,
* the code assumes that unassigned routing tables are
* associated with the default VRF.
*/
info = route_table_get_info(zvrf->table[afi][safi]);
info->zvrf = vrf_info_lookup(VRF_DEFAULT);

rn = route_top(zvrf->table[afi][safi]);
if (rn)
empty_table = false;
while (rn) {
if (!rn->info) {
rn = route_next(rn);
continue;
}

/* Assign the route entries to the default VRF,
* even though they are not actually owned by it.
*/
RNODE_FOREACH_RE (rn, re)
nexthop_vrf_update(rn, re, VRF_DEFAULT);

rn = route_next(rn);
}

if (empty_table)
zebra_router_release_table(zvrf, zvrf->table_id, afi, safi);
zvrf->table[afi][safi] = NULL;
}

/* Callback upon disabling a VRF. */
static int zebra_vrf_disable(struct vrf *vrf)
{
Expand Down Expand Up @@ -216,9 +256,15 @@ static int zebra_vrf_disable(struct vrf *vrf)
* we no-longer need this pointer.
*/
for (safi = SAFI_UNICAST; safi <= SAFI_MULTICAST; safi++) {
zebra_router_release_table(zvrf, zvrf->table_id, afi,
safi);
zvrf->table[afi][safi] = NULL;
if (!zvrf->table[afi][safi] ||
vrf->vrf_id == VRF_DEFAULT) {
zebra_router_release_table(zvrf, zvrf->table_id,
afi, safi);
zvrf->table[afi][safi] = NULL;
continue;
}

zebra_vrf_disable_update_vrfid(zvrf, afi, safi);
}
}

Expand Down Expand Up @@ -349,14 +395,42 @@ static void zebra_rnhtable_node_cleanup(struct route_table *table,
static void zebra_vrf_table_create(struct zebra_vrf *zvrf, afi_t afi,
safi_t safi)
{
vrf_id_t vrf_id = zvrf->vrf->vrf_id;
struct rib_table_info *info;
struct route_entry *re;
struct route_node *rn;
struct prefix p;

assert(!zvrf->table[afi][safi]);

/* Attempt to retrieve the Linux routing table using zvrf->table_id.
* If the table was created before the VRF, it will already exist.
* Otherwise, create a new table.
*/
zvrf->table[afi][safi] =
zebra_router_get_table(zvrf, zvrf->table_id, afi, safi);

/* If the table existed before the VRF was created, info->zvrf was
* referring to the default VRF.
* Assign the table to the new VRF.
* Note: FRR does not allow multiple VRF interfaces to be created with the
* same table ID.
*/
info = route_table_get_info(zvrf->table[afi][safi]);
info->zvrf = zvrf;

/* If the table existed before the VRF was created, their routing entries
* was owned by the default VRF.
* Re-assign all the routing entries to the new VRF.
*/
for (rn = route_top(zvrf->table[afi][safi]); rn; rn = route_next(rn)) {
if (!rn->info)
continue;

RNODE_FOREACH_RE (rn, re)
nexthop_vrf_update(rn, re, vrf_id);
}

memset(&p, 0, sizeof(p));
p.family = afi2family(afi);

Expand Down

0 comments on commit abd5dc7

Please sign in to comment.