bgpd: Prevent usage after free in bgp_mac.c #5260
Conversation
Running with --enable-address-sanitizer I am seeing this:
=================================================================
==19520==ERROR: AddressSanitizer: heap-use-after-free on address 0x6020003ef850 at pc 0x7fe9b8f7b57b bp 0x7fffbac6f9c0 sp 0x7fffbac6f170
READ of size 6 at 0x6020003ef850 thread T0
#0 0x7fe9b8f7b57a (/lib/x86_64-linux-gnu/libasan.so.5+0xb857a)
#1 0x55e33d1071e5 in bgp_process_mac_rescan_table bgpd/bgp_mac.c:159
#2 0x55e33d107c09 in bgp_mac_rescan_evpn_table bgpd/bgp_mac.c:252
#3 0x55e33d107e39 in bgp_mac_rescan_all_evpn_tables bgpd/bgp_mac.c:266
#4 0x55e33d108270 in bgp_mac_remove_ifp_internal bgpd/bgp_mac.c:291
#5 0x55e33d108893 in bgp_mac_del_mac_entry bgpd/bgp_mac.c:351
#6 0x55e33d21412d in bgp_ifp_down bgpd/bgp_zebra.c:257
#7 0x7fe9b8cbf3be in if_down_via_zapi lib/if.c:198
#8 0x7fe9b8db303a in zclient_interface_down lib/zclient.c:1549
#9 0x7fe9b8db8a06 in zclient_read lib/zclient.c:2693
#10 0x7fe9b8d7b95a in thread_call lib/thread.c:1599
#11 0x7fe9b8cd824e in frr_run lib/libfrr.c:1024
#12 0x55e33d09d463 in main bgpd/bgp_main.c:477
#13 0x7fe9b879409a in __libc_start_main ../csu/libc-start.c:308
#14 0x55e33d09c189 in _start (/usr/lib/frr/bgpd+0x168189)
0x6020003ef850 is located 0 bytes inside of 16-byte region [0x6020003ef850,0x6020003ef860)
freed by thread T0 here:
#0 0x7fe9b8fabfb0 in __interceptor_free (/lib/x86_64-linux-gnu/libasan.so.5+0xe8fb0)
#1 0x7fe9b8ce4ea9 in qfree lib/memory.c:129
#2 0x55e33d10825c in bgp_mac_remove_ifp_internal bgpd/bgp_mac.c:289
#3 0x55e33d108893 in bgp_mac_del_mac_entry bgpd/bgp_mac.c:351
#4 0x55e33d21412d in bgp_ifp_down bgpd/bgp_zebra.c:257
#5 0x7fe9b8cbf3be in if_down_via_zapi lib/if.c:198
#6 0x7fe9b8db303a in zclient_interface_down lib/zclient.c:1549
#7 0x7fe9b8db8a06 in zclient_read lib/zclient.c:2693
#8 0x7fe9b8d7b95a in thread_call lib/thread.c:1599
#9 0x7fe9b8cd824e in frr_run lib/libfrr.c:1024
#10 0x55e33d09d463 in main bgpd/bgp_main.c:477
#11 0x7fe9b879409a in __libc_start_main ../csu/libc-start.c:308
previously allocated by thread T0 here:
#0 0x7fe9b8fac518 in calloc (/lib/x86_64-linux-gnu/libasan.so.5+0xe9518)
#1 0x7fe9b8ce4d93 in qcalloc lib/memory.c:110
#2 0x55e33d106b29 in bgp_mac_hash_alloc bgpd/bgp_mac.c:96
#3 0x7fe9b8cb8350 in hash_get lib/hash.c:149
#4 0x55e33d10845b in bgp_mac_add_mac_entry bgpd/bgp_mac.c:303
#5 0x55e33d226757 in bgp_ifp_create bgpd/bgp_zebra.c:2644
#6 0x7fe9b8cbf1e6 in if_new_via_zapi lib/if.c:176
#7 0x7fe9b8db2d3b in zclient_interface_add lib/zclient.c:1481
#8 0x7fe9b8db87f8 in zclient_read lib/zclient.c:2659
#9 0x7fe9b8d7b95a in thread_call lib/thread.c:1599
#10 0x7fe9b8cd824e in frr_run lib/libfrr.c:1024
#11 0x55e33d09d463 in main bgpd/bgp_main.c:477
#12 0x7fe9b879409a in __libc_start_main ../csu/libc-start.c:308
Effectively we are passing to bgp_mac_remove_ifp_internal the macaddr
that is associated with the bsm data structure. There exists a path
where the bsm is freed and then we immediately pass the macaddr into
bgp_mac_rescan_all_evpn_tables. So just make a copy of the macaddr
data structure before we free the bsm
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
| hash_release(bm->self_mac_hash, bsm); | ||
| list_delete(&bsm->ifp_list); | ||
| XFREE(MTYPE_BSM, bsm); | ||
|
|
||
| bgp_mac_rescan_all_evpn_tables(macaddr); | ||
| bgp_mac_rescan_all_evpn_tables(&mac); |
There was a problem hiding this comment.
what if we move bgp_mac_rescan_all_evpn_tables() call to before XFREE and/or list_delete instead of making a copy and use it after free ?
There was a problem hiding this comment.
I cannot say exactly what bgp_mac_rescan_all_evpn_tables does. I would prefer to not have to think that through. not sure we are saving much here though by making that change.
There was a problem hiding this comment.
I can check that for you. Not about saving anything, trying to avoid the usage after it is freed.
There was a problem hiding this comment.
@donaldsharp I checked the code flow for function bgp_mac_rescan_all_evpn_tables and usage of passed macaddr.
bgp_mac_rescan_all_evpn_tables ==> bgp_mac_rescan_evpn_table ==> bgp_process_mac_rescan_table == > mac address is used to compare and check if any MAC/IP route is ever advertised and if so marked the rn for further processing. So I don't see any thing wrong to move this call to before XFREE, instead of making a copy of this.
There was a problem hiding this comment.
I think it makes sense what @srimohans says (just looked over the code what's going on how bgp_mac_remove_ifp_internal() is called from other places).
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-9571/ This is a comment from an automated CI system. CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base1 Static Analyzer issues remaining.See details at |
Running with --enable-address-sanitizer I am seeing this:
Effectively we are passing to bgp_mac_remove_ifp_internal the macaddr
that is associated with the bsm data structure. There exists a path
where the bsm is freed and then we immediately pass the macaddr into
bgp_mac_rescan_all_evpn_tables. So just make a copy of the macaddr
data structure before we free the bsm
Signed-off-by: Donald Sharp sharpd@cumulusnetworks.com