[fdborch] fix heap-use-after-free in clearFdbEntry() #2353
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* using a copy of FDBEntry fields (stored in FDBUpdate) instead of a reference since the reference gets invalidated in the storeFdbEntryState() * simplified clearFdbEntry() interface Signed-off-by: Yakiv Huryk <yhuryk@nvidia.com>
oleksandrivantsiv
approved these changes
Jun 24, 2022
liat-grozovik
approved these changes
Jun 24, 2022
|
Request for 202111 |
yxieca
pushed a commit
that referenced
this pull request
Jun 25, 2022
- What I did using a copy of FDBEntry fields (stored in FDBUpdate) instead of a reference since the reference gets invalidated in the storeFdbEntryState() simplified clearFdbEntry() interface - Why I did it To fix the memory usage issue The issue is that the SWSS_LOG_INFO() uses the mac&, port_alias&, and bv_id& which are invalidated in the storeFdbEntryState(). - How I verified it Run the tests that were used to find the issues and checked the ASAN report Signed-off-by: Yakiv Huryk <yhuryk@nvidia.com>
7 tasks
jimmyzhai
added a commit
to sonic-net/sonic-buildimage
that referenced
this pull request
Jun 27, 2022
2022-06-24 93af69c: [PFC_WD] Avoid applying ZeroBuffer Profiles to ingress PG when a PFC storm is detected (sonic-net/sonic-swss#2304) 2022-06-24 37349cf: [swssconfig] Optimize performance of swssconfig (sonic-net/sonic-swss#2336) 2022-06-24 84e9b07: [fdborch] fix heap-use-after-free in clearFdbEntry() (sonic-net/sonic-swss#2353) 2022-06-24 1b8bd94: Create ACL table fails due to incorrect check for supported ACL actions #11235 (sonic-net/sonic-swss#2351) 2022-06-24 1ed0b4b: [macsec] Refactor the logic of macsec name map (sonic-net/sonic-swss#2348) 2022-06-23 f88f992: [mock_tests] Add Sflow Orch UTs (sonic-net/sonic-swss#2295) 2022-06-23 ec57bf1: [macsec] Update macsec flex counter (sonic-net/sonic-swss#2338) 2022-06-22 6e0fc85: [ACL] Support stage particular match fields (sonic-net/sonic-swss#2341) 2022-06-22 efb4530: [orchagent, DTel]: report session support to set user vrf (sonic-net/sonic-swss#2326) 2022-06-22 d82874d: Fix for "orchagent crashed when trying to delete fdb static entry with swssconfig #11046" (sonic-net/sonic-swss#2332) 2022-06-22 0c789e6: Fix qos map test in vs test (sonic-net/sonic-swss#2343) 2022-06-17 1bb5070: Enhance mock test for dynamic buffer manager for port removing and qos reload flows (sonic-net/sonic-swss#2262) 2022-06-16 700492f: [aclorch] Fix and simplify DTel watchlist tables and entries (sonic-net/sonic-swss#2155)
vivekrnv
pushed a commit
to vivekrnv/sonic-swss
that referenced
this pull request
Jul 29, 2022
- What I did using a copy of FDBEntry fields (stored in FDBUpdate) instead of a reference since the reference gets invalidated in the storeFdbEntryState() simplified clearFdbEntry() interface - Why I did it To fix the memory usage issue The issue is that the SWSS_LOG_INFO() uses the mac&, port_alias&, and bv_id& which are invalidated in the storeFdbEntryState(). - How I verified it Run the tests that were used to find the issues and checked the ASAN report Signed-off-by: Yakiv Huryk <yhuryk@nvidia.com>
preetham-singh
pushed a commit
to preetham-singh/sonic-swss
that referenced
this pull request
Aug 6, 2022
- What I did using a copy of FDBEntry fields (stored in FDBUpdate) instead of a reference since the reference gets invalidated in the storeFdbEntryState() simplified clearFdbEntry() interface - Why I did it To fix the memory usage issue The issue is that the SWSS_LOG_INFO() uses the mac&, port_alias&, and bv_id& which are invalidated in the storeFdbEntryState(). - How I verified it Run the tests that were used to find the issues and checked the ASAN report Signed-off-by: Yakiv Huryk <yhuryk@nvidia.com>
lukasstockner
pushed a commit
to genesiscloud/sonic-swss
that referenced
this pull request
Mar 31, 2023
- What I did using a copy of FDBEntry fields (stored in FDBUpdate) instead of a reference since the reference gets invalidated in the storeFdbEntryState() simplified clearFdbEntry() interface - Why I did it To fix the memory usage issue The issue is that the SWSS_LOG_INFO() uses the mac&, port_alias&, and bv_id& which are invalidated in the storeFdbEntryState(). - How I verified it Run the tests that were used to find the issues and checked the ASAN report Signed-off-by: Yakiv Huryk <yhuryk@nvidia.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The issue is that the SWSS_LOG_INFO() uses the mac&, port_alias&, and bv_id& which are invalidated in the storeFdbEntryState().
ASAN report:
fdborch
==38==ERROR: AddressSanitizer: heap-use-after-free on address 0x611000293668 at pc 0x556914460329 bp 0x7ffc930d0dc0 sp 0x7ffc930d0db8 READ of size 8 at 0x611000293668 thread T0 #0 0x556914460328 in FdbOrch::clearFdbEntry(swss::MacAddress const&, unsigned long const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) orchagent/fdborch.cpp:203 #1 0x556914461443 in FdbOrch::handleSyncdFlushNotif(unsigned long const&, unsigned long const&, swss::MacAddress const&) orchagent/fdborch.cpp:271 #2 0x55691446302a in FdbOrch::update(_sai_fdb_event_t, _sai_fdb_entry_t const*, unsigned long) orchagent/fdborch.cpp:598 #3 0x556914467c50 in FdbOrch::doTask(swss::NotificationConsumer&) orchagent/fdborch.cpp:1010 #4 0x556913fd57e8 in OrchDaemon::start() orchagent/orchdaemon.cpp:723 #5 0x556913e20135 in main orchagent/main.cpp:734 #6 0x7f5f1ca66d09 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x26d09) #7 0x556913f706b9 (/usr/bin/orchagent+0x32d6b9) 0x611000293668 is located 40 bytes inside of 200-byte region [0x611000293640,0x611000293708) freed by thread T0 here: #0 0x7f5f1d510467 in operator delete(void*, unsigned long) ../../../../src/libsanitizer/asan/asan_new_delete.cpp:172 #1 0x55691447bd37 in __gnu_cxx::new_allocator<std::_Rb_tree_node<std::pair<FdbEntry const, FdbData> > >::deallocate(std::_Rb_tree_node<std::pair<FdbEntry const, FdbData> >*, unsigned long) /usr/include/c++/10/ext/new_allocator.h:133 #2 0x55691447bd37 in std::allocator_traits<std::allocator<std::_Rb_tree_node<std::pair<FdbEntry const, FdbData> > > >::deallocate(std::allocator<std::_Rb_tree_node<std::pair<FdbEntry const, FdbData> > >&, std::_Rb_tree_node<std::pair<FdbEntry const, FdbData> >*, unsigned long) /usr/include/c++/10/bits/alloc_traits.h:492 #3 0x55691447bd37 in std::_Rb_tree<FdbEntry, std::pair<FdbEntry const, FdbData>, std::_Select1st<std::pair<FdbEntry const, FdbData> >, std::less<FdbEntry>, std::allocator<std::pair<FdbEntry const, FdbData> > >::_M_put_node(std::_Rb_tree_node<std::pair<FdbEntry const, FdbData> >*) /usr/include/c++/10/bits/stl_tree.h:588 #4 0x55691447bd37 in std::_Rb_tree<FdbEntry, std::pair<FdbEntry const, FdbData>, std::_Select1st<std::pair<FdbEntry const, FdbData> >, std::less<FdbEntry>, std::allocator<std::pair<FdbEntry const, FdbData> > >::_M_drop_node(std::_Rb_tree_node<std::pair<FdbEntry const, FdbData> >*) /usr/include/c++/10/bits/stl_tree.h:655 #5 0x55691447bd37 in std::_Rb_tree<FdbEntry, std::pair<FdbEntry const, FdbData>, std::_Select1st<std::pair<FdbEntry const, FdbData> >, std::less<FdbEntry>, std::allocator<std::pair<FdbEntry const, FdbData> > >::_M_erase_aux(std::_Rb_tree_const_iterator<std::pair<FdbEntry const, FdbData> >) /usr/include/c++/10/bits/stl_tree.h:2517 #6 0x55691447bd37 in std::_Rb_tree<FdbEntry, std::pair<FdbEntry const, FdbData>, std::_Select1st<std::pair<FdbEntry const, FdbData> >, std::less<FdbEntry>, std::allocator<std::pair<FdbEntry const, FdbData> > >::_M_erase_aux(std::_Rb_tree_const_iterator<std::pair<FdbEntry const, FdbData> >, std::_Rb_tree_const_iterator<std::pair<FdbEntry const, FdbData> >) /usr/include/c++/10/bits/stl_tree.h:2531 #7 0x5569144558a1 in std::_Rb_tree<FdbEntry, std::pair<FdbEntry const, FdbData>, std::_Select1st<std::pair<FdbEntry const, FdbData> >, std::less<FdbEntry>, std::allocator<std::pair<FdbEntry const, FdbData> > >::erase(FdbEntry const&) /usr/include/c++/10/bits/stl_tree.h:2542 #8 0x5569144558a1 in std::map<FdbEntry, FdbData, std::less<FdbEntry>, std::allocator<std::pair<FdbEntry const, FdbData> > >::erase(FdbEntry const&) /usr/include/c++/10/bits/stl_map.h:1069 #9 0x5569144558a1 in FdbOrch::storeFdbEntryState(FdbUpdate const&) orchagent/fdborch.cpp:150 #10 0x55691445fe5a in FdbOrch::clearFdbEntry(swss::MacAddress const&, unsigned long const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) orchagent/fdborch.cpp:200 #11 0x556914461443 in FdbOrch::handleSyncdFlushNotif(unsigned long const&, unsigned long const&, swss::MacAddress const&) orchagent/fdborch.cpp:271 #12 0x55691446302a in FdbOrch::update(_sai_fdb_event_t, _sai_fdb_entry_t const*, unsigned long) orchagent/fdborch.cpp:598 #13 0x556914467c50 in FdbOrch::doTask(swss::NotificationConsumer&) orchagent/fdborch.cpp:1010 #14 0x556913fd57e8 in OrchDaemon::start() orchagent/orchdaemon.cpp:723 #15 0x556913e20135 in main orchagent/main.cpp:734 #16 0x7f5f1ca66d09 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x26d09) previously allocated by thread T0 here: #0 0x7f5f1d50f647 in operator new(unsigned long) ../../../../src/libsanitizer/asan/asan_new_delete.cpp:99 #1 0x556914480bbb in __gnu_cxx::new_allocator<std::_Rb_tree_node<std::pair<FdbEntry const, FdbData> > >::allocate(unsigned long, void const*) /usr/include/c++/10/ext/new_allocator.h:115 #2 0x556914480bbb in std::allocator_traits<std::allocator<std::_Rb_tree_node<std::pair<FdbEntry const, FdbData> > > >::allocate(std::allocator<std::_Rb_tree_node<std::pair<FdbEntry const, FdbData> > >&, unsigned long) /usr/include/c++/10/bits/alloc_traits.h:460 #3 0x556914480bbb in std::_Rb_tree<FdbEntry, std::pair<FdbEntry const, FdbData>, std::_Select1st<std::pair<FdbEntry const, FdbData> >, std::less<FdbEntry>, std::allocator<std::pair<FdbEntry const, FdbData> > >::_M_get_node() /usr/include/c++/10/bits/stl_tree.h:584 #4 0x556914480bbb in std::_Rb_tree_node<std::pair<FdbEntry const, FdbData> >* std::_Rb_tree<FdbEntry, std::pair<FdbEntry const, FdbData>, std::_Select1st<std::pair<FdbEntry const, FdbData> >, std::less<FdbEntry>, std::allocator<std::pair<FdbEntry const, FdbData> > >::_M_create_node<std::piecewise_construct_t const&, std::tuple<FdbEntry const&>, std::tuple<> >(std::piecewise_construct_t const&, std::tuple<FdbEntry const&>&&, std::tuple<>&&) /usr/include/c++/10/bits/stl_tree.h:634 #5 0x556914480bbb in std::_Rb_tree_iterator<std::pair<FdbEntry const, FdbData> > std::_Rb_tree<FdbEntry, std::pair<FdbEntry const, FdbData>, std::_Select1st<std::pair<FdbEntry const, FdbData> >, std::less<FdbEntry>, std::allocator<std::pair<FdbEntry const, FdbData> > >::_M_emplace_hint_unique<std::piecewise_construct_t const&, std::tuple<FdbEntry const&>, std::tuple<> >(std::_Rb_tree_const_iterator<std::pair<FdbEntry const, FdbData> >, std::piecewise_construct_t const&, std::tuple<FdbEntry const&>&&, std::tuple<>&&) /usr/include/c++/10/bits/stl_tree.h:2461 #6 0x556914480bbb in std::map<FdbEntry, FdbData, std::less<FdbEntry>, std::allocator<std::pair<FdbEntry const, FdbData> > >::operator[](FdbEntry const&) /usr/include/c++/10/bits/stl_map.h:501 #7 0x55691445462d in FdbOrch::storeFdbEntryState(FdbUpdate const&) orchagent/fdborch.cpp:117 #8 0x5569144652bc in FdbOrch::update(_sai_fdb_event_t, _sai_fdb_entry_t const*, unsigned long) orchagent/fdborch.cpp:411 #9 0x556914467c50 in FdbOrch::doTask(swss::NotificationConsumer&) orchagent/fdborch.cpp:1010 #10 0x556913fd57e8 in OrchDaemon::start() orchagent/orchdaemon.cpp:723 #11 0x556913e20135 in main orchagent/main.cpp:734 #12 0x7f5f1ca66d09 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x26d09) SUMMARY: AddressSanitizer: heap-use-after-free orchagent/fdborch.cpp:203 in FdbOrch::clearFdbEntry(swss::MacAddress const&, unsigned long const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) Shadow bytes around the buggy address: 0x0c228004a670: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd 0x0c228004a680: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c228004a690: fd fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c228004a6a0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c228004a6b0: fd fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa =>0x0c228004a6c0: fa fa fa fa fa fa fa fa fd fd fd fd fd[fd]fd fd 0x0c228004a6d0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c228004a6e0: fd fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c228004a6f0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c228004a700: fd fd fd fd fd fd fd fd fd fd fd fa fa fa fa fa 0x0c228004a710: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb Shadow gap: cc ==38==ABORTINGWhat I did
Why I did it
To fix the memory usage issue
How I verified it
Run the tests that were used to find the issues and checked the ASAN report
Details if related