Skip to content

Commit

Permalink
bgpd: Use bgp_session_reset_safe() for GR update all peers
Browse files Browse the repository at this point in the history 
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
    FRRouting#3 0x55f3ab74db69 in bgp_graceful_restart bgpd/bgp_vty.c:3130
    FRRouting#4 0x7fc5539a9584 in cmd_execute_command_real lib/command.c:1002
    FRRouting#5 0x7fc5539a98a3 in cmd_execute_command lib/command.c:1061
    FRRouting#6 0x7fc5539a9dcf in cmd_execute lib/command.c:1227
    FRRouting#7 0x7fc553ae493f in vty_command lib/vty.c:616
    FRRouting#8 0x7fc553ae4e92 in vty_execute lib/vty.c:1379
    FRRouting#9 0x7fc553aedd34 in vtysh_read lib/vty.c:2374
    FRRouting#10 0x7fc553ad8a64 in event_call lib/event.c:1995
    FRRouting#11 0x7fc553a0c429 in frr_run lib/libfrr.c:1232
    FRRouting#12 0x55f3ab57b78d in main bgpd/bgp_main.c:555
    FRRouting#13 0x7fc55342d249 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    FRRouting#14 0x7fc55342d304 in __libc_start_main_impl ../csu/libc-start.c:360
    FRRouting#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
    FRRouting#3 0x7fc553a0e50d in list_delete_node lib/linklist.c:379
    FRRouting#4 0x55f3ab7ae353 in peer_delete bgpd/bgpd.c:2796
    FRRouting#5 0x55f3ab7ae91f in bgp_session_reset bgpd/bgpd.c:141
    FRRouting#6 0x55f3ab62ab17 in bgp_gr_update_mode_of_all_peers bgpd/bgp_fsm.c:2752
    FRRouting#7 0x55f3ab62ab17 in bgp_gr_update_all bgpd/bgp_fsm.c:2779
    FRRouting#8 0x55f3ab73557e in bgp_inst_gr_config_vty bgpd/bgp_vty.c:3037
    FRRouting#9 0x55f3ab74db69 in bgp_graceful_restart bgpd/bgp_vty.c:3130
    FRRouting#10 0x7fc5539a9584 in cmd_execute_command_real lib/command.c:1002
    FRRouting#11 0x7fc5539a98a3 in cmd_execute_command lib/command.c:1061
    FRRouting#12 0x7fc5539a9dcf in cmd_execute lib/command.c:1227
    FRRouting#13 0x7fc553ae493f in vty_command lib/vty.c:616
    FRRouting#14 0x7fc553ae4e92 in vty_execute lib/vty.c:1379
    FRRouting#15 0x7fc553aedd34 in vtysh_read lib/vty.c:2374
    FRRouting#16 0x7fc553ad8a64 in event_call lib/event.c:1995
    FRRouting#17 0x7fc553a0c429 in frr_run lib/libfrr.c:1232
    FRRouting#18 0x55f3ab57b78d in main bgpd/bgp_main.c:555
    FRRouting#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
    FRRouting#3 0x7fc553a0d85b in listnode_add_sort lib/linklist.c:197
    FRRouting#4 0x55f3ab7baec4 in peer_create bgpd/bgpd.c:1996
    FRRouting#5 0x55f3ab65be8b in bgp_accept bgpd/bgp_network.c:604
    FRRouting#6 0x7fc553ad8a64 in event_call lib/event.c:1995
    FRRouting#7 0x7fc553a0c429 in frr_run lib/libfrr.c:1232
    FRRouting#8 0x55f3ab57b78d in main bgpd/bgp_main.c:555
    FRRouting#9 0x7fc55342d249 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
```

Signed-off-by: Donatas Abraitis <[email protected]>
  • Loading branch information
@ton31337
ton31337 committed Jul 31, 2024
1 parent 26eceb2 commit 9c710ee
Show file tree
Hide file tree
Showing 3 changed files with 4 additions and 2 deletions.
2 changes: 1 addition & 1 deletion bgpd/bgp_fsm.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down
2 changes: 1 addition & 1 deletion bgpd/bgpd.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 2 additions & 0 deletions bgpd/bgpd.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 *)
Expand Down

0 comments on commit 9c710ee

Please sign in to comment.