From 9c710eef0c08b334c0591addb30b8cdcf509f03f Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Wed, 31 Jul 2024 11:43:19 +0300 Subject: [PATCH] bgpd: Use bgp_session_reset_safe() for GR update all peers It might cause this use-after-free: ``` ==6523==ERROR: AddressSanitizer: heap-use-after-free on address 0x60300058d720 at pc 0x55f3ab62ab1f bp 0x7ffe5b95a0d0 sp 0x7ffe5b95a0c8 READ of size 8 at 0x60300058d720 thread T0 #0 0x55f3ab62ab1e in bgp_gr_update_mode_of_all_peers bgpd/bgp_fsm.c:2729 #1 0x55f3ab62ab1e in bgp_gr_update_all bgpd/bgp_fsm.c:2779 #2 0x55f3ab73557e in bgp_inst_gr_config_vty bgpd/bgp_vty.c:3037 #3 0x55f3ab74db69 in bgp_graceful_restart bgpd/bgp_vty.c:3130 #4 0x7fc5539a9584 in cmd_execute_command_real lib/command.c:1002 #5 0x7fc5539a98a3 in cmd_execute_command lib/command.c:1061 #6 0x7fc5539a9dcf in cmd_execute lib/command.c:1227 #7 0x7fc553ae493f in vty_command lib/vty.c:616 #8 0x7fc553ae4e92 in vty_execute lib/vty.c:1379 #9 0x7fc553aedd34 in vtysh_read lib/vty.c:2374 #10 0x7fc553ad8a64 in event_call lib/event.c:1995 #11 0x7fc553a0c429 in frr_run lib/libfrr.c:1232 #12 0x55f3ab57b78d in main bgpd/bgp_main.c:555 #13 0x7fc55342d249 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 #14 0x7fc55342d304 in __libc_start_main_impl ../csu/libc-start.c:360 #15 0x55f3ab5799a0 in _start (/usr/lib/frr/bgpd+0x2e19a0) 0x60300058d720 is located 16 bytes inside of 24-byte region [0x60300058d710,0x60300058d728) freed by thread T0 here: #0 0x7fc553eb76a8 in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:52 #1 0x7fc553a2b713 in qfree lib/memory.c:130 #2 0x7fc553a0e50d in listnode_free lib/linklist.c:81 #3 0x7fc553a0e50d in list_delete_node lib/linklist.c:379 #4 0x55f3ab7ae353 in peer_delete bgpd/bgpd.c:2796 #5 0x55f3ab7ae91f in bgp_session_reset bgpd/bgpd.c:141 #6 0x55f3ab62ab17 in bgp_gr_update_mode_of_all_peers bgpd/bgp_fsm.c:2752 #7 0x55f3ab62ab17 in bgp_gr_update_all bgpd/bgp_fsm.c:2779 #8 0x55f3ab73557e in bgp_inst_gr_config_vty bgpd/bgp_vty.c:3037 #9 0x55f3ab74db69 in bgp_graceful_restart bgpd/bgp_vty.c:3130 #10 0x7fc5539a9584 in cmd_execute_command_real lib/command.c:1002 #11 0x7fc5539a98a3 in cmd_execute_command lib/command.c:1061 #12 0x7fc5539a9dcf in cmd_execute lib/command.c:1227 #13 0x7fc553ae493f in vty_command lib/vty.c:616 #14 0x7fc553ae4e92 in vty_execute lib/vty.c:1379 #15 0x7fc553aedd34 in vtysh_read lib/vty.c:2374 #16 0x7fc553ad8a64 in event_call lib/event.c:1995 #17 0x7fc553a0c429 in frr_run lib/libfrr.c:1232 #18 0x55f3ab57b78d in main bgpd/bgp_main.c:555 #19 0x7fc55342d249 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 previously allocated by thread T0 here: #0 0x7fc553eb83b7 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:77 #1 0x7fc553a2ae20 in qcalloc lib/memory.c:105 #2 0x7fc553a0d056 in listnode_new lib/linklist.c:71 #3 0x7fc553a0d85b in listnode_add_sort lib/linklist.c:197 #4 0x55f3ab7baec4 in peer_create bgpd/bgpd.c:1996 #5 0x55f3ab65be8b in bgp_accept bgpd/bgp_network.c:604 #6 0x7fc553ad8a64 in event_call lib/event.c:1995 #7 0x7fc553a0c429 in frr_run lib/libfrr.c:1232 #8 0x55f3ab57b78d in main bgpd/bgp_main.c:555 #9 0x7fc55342d249 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 ``` Signed-off-by: Donatas Abraitis --- bgpd/bgp_fsm.c | 2 +- bgpd/bgpd.c | 2 +- bgpd/bgpd.h | 2 ++ 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/bgpd/bgp_fsm.c b/bgpd/bgp_fsm.c index 1eeb14115519..98ebf5138541 100644 --- a/bgpd/bgp_fsm.c +++ b/bgpd/bgp_fsm.c @@ -2749,7 +2749,7 @@ static void bgp_gr_update_mode_of_all_peers(struct bgp *bgp, bgp_notify_send(peer->connection, BGP_NOTIFY_CEASE, BGP_NOTIFY_CEASE_CONFIG_CHANGE); else - bgp_session_reset(peer); + bgp_session_reset_safe(peer, &nnode); } } diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index b32a4640f02b..44cd24565a96 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -149,7 +149,7 @@ void bgp_session_reset(struct peer *peer) * during walk of peer list, we would end up accessing the freed next * node. This function moves the next node along. */ -static void bgp_session_reset_safe(struct peer *peer, struct listnode **nnode) +void bgp_session_reset_safe(struct peer *peer, struct listnode **nnode) { struct listnode *n; struct peer *npeer; diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h index 6e6358bac78b..b733be9f0dd8 100644 --- a/bgpd/bgpd.h +++ b/bgpd/bgpd.h @@ -2858,6 +2858,8 @@ extern bool bgp_path_attribute_treat_as_withdraw(struct peer *peer, char *buf, extern void srv6_function_free(struct bgp_srv6_function *func); +extern void bgp_session_reset_safe(struct peer *peer, struct listnode **nnode); + #ifdef _FRR_ATTRIBUTE_PRINTFRR /* clang-format off */ #pragma FRR printfrr_ext "%pBP" (struct peer *)