Skip to content

Commit

Permalink
bgpd: Fix read after free in labeled_unicast on shutdown
Browse files Browse the repository at this point in the history
Fix:

==2937693== Invalid read of size 2
==2937693==    at 0x2EFD44: bgp_delete_listnode (bgp_table.c:170)
==2937693==    by 0x2EFB22: bgp_dest_unlock_node (bgp_table.c:81)
==2937693==    by 0x26BAC1: check_bgp_lu_cb_unlock (bgp_labelpool.c:208)
==2937693==    by 0x26BC58: bgp_lp_finish (bgp_labelpool.c:243)
==2937693==    by 0x1FE006: bgp_exit (bgp_main.c:256)
==2937693==    by 0x1FDD99: sigint (bgp_main.c:163)
==2937693==    by 0x49BA8BB: frr_sigevent_process (sigevent.c:117)
==2937693==    by 0x49D5912: event_fetch (event.c:1782)
==2937693==    by 0x4955BBF: frr_run (libfrr.c:1216)
==2937693==    by 0x1FEA22: main (bgp_main.c:543)
==2937693==  Address 0x91af6cc is 60 bytes inside a block of size 96 free'd
==2937693==    at 0x484B27F: free (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2937693==    by 0x4968C57: qfree (memory.c:130)
==2937693==    by 0x2EFC55: bgp_node_destroy (bgp_table.c:116)
==2937693==    by 0x49CB7DB: route_node_free (table.c:76)
==2937693==    by 0x49CB8A2: route_table_free (table.c:111)
==2937693==    by 0x49CB6E5: route_table_finish (table.c:46)
==2937693==    by 0x2EFA15: bgp_table_unlock (bgp_table.c:35)
==2937693==    by 0x2EFA6F: bgp_table_finish (bgp_table.c:44)
==2937693==    by 0x3625FA: bgp_free (bgpd.c:4072)
==2937693==    by 0x353858: bgp_unlock (bgpd.h:2525)
==2937693==    by 0x362411: bgp_delete (bgpd.c:4034)
==2937693==    by 0x1FDF13: bgp_exit (bgp_main.c:205)
==2937693==    by 0x1FDD99: sigint (bgp_main.c:163)
==2937693==    by 0x49BA8BB: frr_sigevent_process (sigevent.c:117)
==2937693==    by 0x49D5912: event_fetch (event.c:1782)
==2937693==    by 0x4955BBF: frr_run (libfrr.c:1216)
==2937693==    by 0x1FEA22: main (bgp_main.c:543)
==2937693==  Block was alloc'd at
==2937693==    at 0x484DA83: calloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2937693==    by 0x4968B05: qcalloc (memory.c:105)
==2937693==    by 0x2A5667: bgp_node_get (bgp_table.h:239)
==2937693==    by 0x2A9C36: bgp_afi_node_get (bgp_route.c:154)
==2937693==    by 0x2B4BA3: bgp_update (bgp_route.c:4225)
==2937693==    by 0x3D5255: bgp_nlri_parse_label (bgp_label.c:452)
==2937693==    by 0x290CBF: bgp_nlri_parse (bgp_packet.c:345)
==2937693==    by 0x29616F: bgp_update_receive (bgp_packet.c:2453)
==2937693==    by 0x29ACEE: bgp_process_packet (bgp_packet.c:4022)
==2937693==    by 0x49D5F18: event_call (event.c:2011)
==2937693==    by 0x4955BA6: frr_run (libfrr.c:1217)
==2937693==    by 0x1FEA22: main (bgp_main.c:543)

This is occurring because the shutdown code is hard deleting the
route node but labeled unicast has a pointer to it.  Free up
the labeled unicast data before hard free'ing the route nodes.

Signed-off-by: Donald Sharp <[email protected]>
  • Loading branch information
donaldsharp committed Nov 4, 2024
1 parent 960462a commit 58499e3
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions bgpd/bgp_main.c
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,8 @@ static __attribute__((__noreturn__)) void bgp_exit(int status)
bgp_default = bgp_get_default();
bgp_evpn = bgp_get_evpn();

bgp_lp_finish();

/* reverse bgp_master_init */
for (ALL_LIST_ELEMENTS(bm->bgp, node, nnode, bgp)) {
if (bgp_default == bgp || bgp_evpn == bgp)
Expand Down Expand Up @@ -257,8 +259,6 @@ static __attribute__((__noreturn__)) void bgp_exit(int status)
list_delete(&bm->bgp);
list_delete(&bm->addresses);

bgp_lp_finish();

memset(bm, 0, sizeof(*bm));

frr_fini();
Expand Down

0 comments on commit 58499e3

Please sign in to comment.