| Message ID | 1681885751-21440-1-git-send-email-wangyunjian@huawei.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] ofproto-dpif-xlate: Fix use-after-free when xlate_actions(). | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/intel-ovs-compilation | success | test: success |
Is there any issue with this patch? Why is it in the Superseded state? > -----Original Message----- > From: wangyunjian > Sent: Wednesday, April 19, 2023 2:29 PM > To: dev@openvswitch.org; i.maximets@ovn.org > Cc: luyicai <luyicai@huawei.com>; wangyunjian <wangyunjian@huawei.com> > Subject: [ovs-dev] [PATCH] ofproto-dpif-xlate: Fix use-after-free when > xlate_actions(). > > Currently, bundle->cvlans and xbundle->cvlans are pointing to the same > memory location. This can cause issues if the main thread modifies > bundle->cvlans and frees it while the revalidator thread is still accessing > xbundle->cvlans. This can result in use after free errors. > > AddressSanitizer: heap-use-after-free on address 0x615000007b08 at pc > 0x0000004ede1e bp 0x7f3120ee0310 sp 0x7f3120ee0300 READ of size 8 at > 0x615000007b08 thread T25 (revalidator25) > #0 0x4ede1d in bitmap_is_set lib/bitmap.h:91 > #1 0x4fcb26 in xbundle_allows_cvlan ofproto/ofproto-dpif-xlate.c:2028 > #2 0x4fe279 in input_vid_is_valid ofproto/ofproto-dpif-xlate.c:2294 > #3 0x502abf in xlate_normal ofproto/ofproto-dpif-xlate.c:3051 > #4 0x5164dc in xlate_output_action ofproto/ofproto-dpif-xlate.c:5361 > #5 0x522576 in do_xlate_actions ofproto/ofproto-dpif-xlate.c:7047 > #6 0x52a751 in xlate_actions ofproto/ofproto-dpif-xlate.c:8061 > #7 0x4e2b66 in xlate_key ofproto/ofproto-dpif-upcall.c:2212 > #8 0x4e2e13 in xlate_ukey ofproto/ofproto-dpif-upcall.c:2227 > #9 0x4e345d in revalidate_ukey__ ofproto/ofproto-dpif-upcall.c:2276 > #10 0x4e3f85 in revalidate_ukey ofproto/ofproto-dpif-upcall.c:2395 > #11 0x4e7ac5 in revalidate ofproto/ofproto-dpif-upcall.c:2858 > #12 0x4d9ed3 in udpif_revalidator ofproto/ofproto-dpif-upcall.c:1010 > #13 0x7cd92e in ovsthread_wrapper lib/ovs-thread.c:423 > #14 0x7f312ff01f3a (/usr/lib64/libpthread.so.0+0x8f3a) > #15 0x7f312fc8f51f in clone (/usr/lib64/libc.so.6+0xf851f) > > 0x615000007b08 is located 8 bytes inside of 512-byte region > [0x615000007b00,0x615000007d00) freed by thread T0 here: > #0 0x7f3130378ad8 in free (/usr/lib64/libasan.so.4+0xe0ad8) > #1 0x49044e in bundle_set ofproto/ofproto-dpif.c:3431 > #2 0x444f92 in ofproto_bundle_register ofproto/ofproto.c:1455 > #3 0x40e6c9 in port_configure vswitchd/bridge.c:1300 > #4 0x40bcfd in bridge_reconfigure vswitchd/bridge.c:921 > #5 0x41f1a9 in bridge_run vswitchd/bridge.c:3313 > #6 0x42d4fb in main vswitchd/ovs-vswitchd.c:132 > #7 0x7f312fbbcc86 in __libc_start_main (/usr/lib64/libc.so.6+0x25c86) > > previously allocated by thread T0 here: > #0 0x7f3130378e70 in __interceptor_malloc > (/usr/lib64/libasan.so.4+0xe0e70) > #1 0x8757fe in xmalloc__ lib/util.c:140 > #2 0x8758da in xmalloc lib/util.c:175 > #3 0x875927 in xmemdup lib/util.c:188 > #4 0x475f63 in bitmap_clone lib/bitmap.h:79 > #5 0x47797c in vlan_bitmap_clone lib/vlan-bitmap.h:40 > #6 0x49048d in bundle_set ofproto/ofproto-dpif.c:3433 > #7 0x444f92 in ofproto_bundle_register ofproto/ofproto.c:1455 > #8 0x40e6c9 in port_configure vswitchd/bridge.c:1300 > #9 0x40bcfd in bridge_reconfigure vswitchd/bridge.c:921 > #10 0x41f1a9 in bridge_run vswitchd/bridge.c:3313 > #11 0x42d4fb in main vswitchd/ovs-vswitchd.c:132 > #12 0x7f312fbbcc86 in __libc_start_main (/usr/lib64/libc.so.6+0x25c86) > > Fixes: fed8962aff57 ("Add new port VLAN mode "dot1q-tunnel"") > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> > --- > ofproto/ofproto-dpif-xlate.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index > c01177718..455ca3dac 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -66,6 +66,7 @@ > #include "tunnel.h" > #include "util.h" > #include "uuid.h" > +#include "vlan-bitmap.h" > > COVERAGE_DEFINE(xlate_actions); > COVERAGE_DEFINE(xlate_actions_oversize); > @@ -1028,7 +1029,12 @@ xlate_xbundle_set(struct xbundle *xbundle, > xbundle->qinq_ethtype = qinq_ethtype; > xbundle->vlan = vlan; > xbundle->trunks = trunks; > - xbundle->cvlans = cvlans; > + if (xbundle->cvlans == NULL) { > + xbundle->cvlans = vlan_bitmap_clone(cvlans); > + } else if (!vlan_bitmap_equal(xbundle->cvlans, cvlans)) { > + ovsrcu_postpone(free, xbundle->cvlans); > + xbundle->cvlans = vlan_bitmap_clone(cvlans); > + } > xbundle->use_priority_tags = use_priority_tags; > xbundle->floodable = floodable; > xbundle->protected = protected; > @@ -1380,6 +1386,7 @@ xlate_xbundle_remove(struct xlate_cfg *xcfg, struct > xbundle *xbundle) > ovs_list_remove(&xbundle->list_node); > bond_unref(xbundle->bond); > lacp_unref(xbundle->lacp); > + free(xbundle->cvlans); > free(xbundle->name); > free(xbundle); > } > -- > 2.27.0
On 4/23/23 05:47, wangyunjian wrote: > Is there any issue with this patch? Why is it in the Superseded state? Hi. Not sure what happened here. I moved it back to 'New' for now. Best regards, Ilya Maximets. > >> -----Original Message----- >> From: wangyunjian >> Sent: Wednesday, April 19, 2023 2:29 PM >> To: dev@openvswitch.org; i.maximets@ovn.org >> Cc: luyicai <luyicai@huawei.com>; wangyunjian <wangyunjian@huawei.com> >> Subject: [ovs-dev] [PATCH] ofproto-dpif-xlate: Fix use-after-free when >> xlate_actions(). >> >> Currently, bundle->cvlans and xbundle->cvlans are pointing to the same >> memory location. This can cause issues if the main thread modifies >> bundle->cvlans and frees it while the revalidator thread is still accessing >> xbundle->cvlans. This can result in use after free errors. >> >> AddressSanitizer: heap-use-after-free on address 0x615000007b08 at pc >> 0x0000004ede1e bp 0x7f3120ee0310 sp 0x7f3120ee0300 READ of size 8 at >> 0x615000007b08 thread T25 (revalidator25) >> #0 0x4ede1d in bitmap_is_set lib/bitmap.h:91 >> #1 0x4fcb26 in xbundle_allows_cvlan ofproto/ofproto-dpif-xlate.c:2028 >> #2 0x4fe279 in input_vid_is_valid ofproto/ofproto-dpif-xlate.c:2294 >> #3 0x502abf in xlate_normal ofproto/ofproto-dpif-xlate.c:3051 >> #4 0x5164dc in xlate_output_action ofproto/ofproto-dpif-xlate.c:5361 >> #5 0x522576 in do_xlate_actions ofproto/ofproto-dpif-xlate.c:7047 >> #6 0x52a751 in xlate_actions ofproto/ofproto-dpif-xlate.c:8061 >> #7 0x4e2b66 in xlate_key ofproto/ofproto-dpif-upcall.c:2212 >> #8 0x4e2e13 in xlate_ukey ofproto/ofproto-dpif-upcall.c:2227 >> #9 0x4e345d in revalidate_ukey__ ofproto/ofproto-dpif-upcall.c:2276 >> #10 0x4e3f85 in revalidate_ukey ofproto/ofproto-dpif-upcall.c:2395 >> #11 0x4e7ac5 in revalidate ofproto/ofproto-dpif-upcall.c:2858 >> #12 0x4d9ed3 in udpif_revalidator ofproto/ofproto-dpif-upcall.c:1010 >> #13 0x7cd92e in ovsthread_wrapper lib/ovs-thread.c:423 >> #14 0x7f312ff01f3a (/usr/lib64/libpthread.so.0+0x8f3a) >> #15 0x7f312fc8f51f in clone (/usr/lib64/libc.so.6+0xf851f) >> >> 0x615000007b08 is located 8 bytes inside of 512-byte region >> [0x615000007b00,0x615000007d00) freed by thread T0 here: >> #0 0x7f3130378ad8 in free (/usr/lib64/libasan.so.4+0xe0ad8) >> #1 0x49044e in bundle_set ofproto/ofproto-dpif.c:3431 >> #2 0x444f92 in ofproto_bundle_register ofproto/ofproto.c:1455 >> #3 0x40e6c9 in port_configure vswitchd/bridge.c:1300 >> #4 0x40bcfd in bridge_reconfigure vswitchd/bridge.c:921 >> #5 0x41f1a9 in bridge_run vswitchd/bridge.c:3313 >> #6 0x42d4fb in main vswitchd/ovs-vswitchd.c:132 >> #7 0x7f312fbbcc86 in __libc_start_main (/usr/lib64/libc.so.6+0x25c86) >> >> previously allocated by thread T0 here: >> #0 0x7f3130378e70 in __interceptor_malloc >> (/usr/lib64/libasan.so.4+0xe0e70) >> #1 0x8757fe in xmalloc__ lib/util.c:140 >> #2 0x8758da in xmalloc lib/util.c:175 >> #3 0x875927 in xmemdup lib/util.c:188 >> #4 0x475f63 in bitmap_clone lib/bitmap.h:79 >> #5 0x47797c in vlan_bitmap_clone lib/vlan-bitmap.h:40 >> #6 0x49048d in bundle_set ofproto/ofproto-dpif.c:3433 >> #7 0x444f92 in ofproto_bundle_register ofproto/ofproto.c:1455 >> #8 0x40e6c9 in port_configure vswitchd/bridge.c:1300 >> #9 0x40bcfd in bridge_reconfigure vswitchd/bridge.c:921 >> #10 0x41f1a9 in bridge_run vswitchd/bridge.c:3313 >> #11 0x42d4fb in main vswitchd/ovs-vswitchd.c:132 >> #12 0x7f312fbbcc86 in __libc_start_main (/usr/lib64/libc.so.6+0x25c86) >> >> Fixes: fed8962aff57 ("Add new port VLAN mode "dot1q-tunnel"") >> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> >> --- >> ofproto/ofproto-dpif-xlate.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index >> c01177718..455ca3dac 100644 >> --- a/ofproto/ofproto-dpif-xlate.c >> +++ b/ofproto/ofproto-dpif-xlate.c >> @@ -66,6 +66,7 @@ >> #include "tunnel.h" >> #include "util.h" >> #include "uuid.h" >> +#include "vlan-bitmap.h" >> >> COVERAGE_DEFINE(xlate_actions); >> COVERAGE_DEFINE(xlate_actions_oversize); >> @@ -1028,7 +1029,12 @@ xlate_xbundle_set(struct xbundle *xbundle, >> xbundle->qinq_ethtype = qinq_ethtype; >> xbundle->vlan = vlan; >> xbundle->trunks = trunks; >> - xbundle->cvlans = cvlans; >> + if (xbundle->cvlans == NULL) { >> + xbundle->cvlans = vlan_bitmap_clone(cvlans); >> + } else if (!vlan_bitmap_equal(xbundle->cvlans, cvlans)) { >> + ovsrcu_postpone(free, xbundle->cvlans); >> + xbundle->cvlans = vlan_bitmap_clone(cvlans); >> + } >> xbundle->use_priority_tags = use_priority_tags; >> xbundle->floodable = floodable; >> xbundle->protected = protected; >> @@ -1380,6 +1386,7 @@ xlate_xbundle_remove(struct xlate_cfg *xcfg, struct >> xbundle *xbundle) >> ovs_list_remove(&xbundle->list_node); >> bond_unref(xbundle->bond); >> lacp_unref(xbundle->lacp); >> + free(xbundle->cvlans); >> free(xbundle->name); >> free(xbundle); >> } >> -- >> 2.27.0 >
On Wed, Apr 19, 2023 at 02:29:11PM +0800, Yunjian Wang via dev wrote: > Currently, bundle->cvlans and xbundle->cvlans are pointing to the > same memory location. This can cause issues if the main thread > modifies bundle->cvlans and frees it while the revalidator thread > is still accessing xbundle->cvlans. This can result in use after > free errors. > > AddressSanitizer: heap-use-after-free on address 0x615000007b08 at pc 0x0000004ede1e bp 0x7f3120ee0310 sp 0x7f3120ee0300 > READ of size 8 at 0x615000007b08 thread T25 (revalidator25) > #0 0x4ede1d in bitmap_is_set lib/bitmap.h:91 > #1 0x4fcb26 in xbundle_allows_cvlan ofproto/ofproto-dpif-xlate.c:2028 > #2 0x4fe279 in input_vid_is_valid ofproto/ofproto-dpif-xlate.c:2294 > #3 0x502abf in xlate_normal ofproto/ofproto-dpif-xlate.c:3051 > #4 0x5164dc in xlate_output_action ofproto/ofproto-dpif-xlate.c:5361 > #5 0x522576 in do_xlate_actions ofproto/ofproto-dpif-xlate.c:7047 > #6 0x52a751 in xlate_actions ofproto/ofproto-dpif-xlate.c:8061 > #7 0x4e2b66 in xlate_key ofproto/ofproto-dpif-upcall.c:2212 > #8 0x4e2e13 in xlate_ukey ofproto/ofproto-dpif-upcall.c:2227 > #9 0x4e345d in revalidate_ukey__ ofproto/ofproto-dpif-upcall.c:2276 > #10 0x4e3f85 in revalidate_ukey ofproto/ofproto-dpif-upcall.c:2395 > #11 0x4e7ac5 in revalidate ofproto/ofproto-dpif-upcall.c:2858 > #12 0x4d9ed3 in udpif_revalidator ofproto/ofproto-dpif-upcall.c:1010 > #13 0x7cd92e in ovsthread_wrapper lib/ovs-thread.c:423 > #14 0x7f312ff01f3a (/usr/lib64/libpthread.so.0+0x8f3a) > #15 0x7f312fc8f51f in clone (/usr/lib64/libc.so.6+0xf851f) > > 0x615000007b08 is located 8 bytes inside of 512-byte region [0x615000007b00,0x615000007d00) > freed by thread T0 here: > #0 0x7f3130378ad8 in free (/usr/lib64/libasan.so.4+0xe0ad8) > #1 0x49044e in bundle_set ofproto/ofproto-dpif.c:3431 > #2 0x444f92 in ofproto_bundle_register ofproto/ofproto.c:1455 > #3 0x40e6c9 in port_configure vswitchd/bridge.c:1300 > #4 0x40bcfd in bridge_reconfigure vswitchd/bridge.c:921 > #5 0x41f1a9 in bridge_run vswitchd/bridge.c:3313 > #6 0x42d4fb in main vswitchd/ovs-vswitchd.c:132 > #7 0x7f312fbbcc86 in __libc_start_main (/usr/lib64/libc.so.6+0x25c86) > > previously allocated by thread T0 here: > #0 0x7f3130378e70 in __interceptor_malloc (/usr/lib64/libasan.so.4+0xe0e70) > #1 0x8757fe in xmalloc__ lib/util.c:140 > #2 0x8758da in xmalloc lib/util.c:175 > #3 0x875927 in xmemdup lib/util.c:188 > #4 0x475f63 in bitmap_clone lib/bitmap.h:79 > #5 0x47797c in vlan_bitmap_clone lib/vlan-bitmap.h:40 > #6 0x49048d in bundle_set ofproto/ofproto-dpif.c:3433 > #7 0x444f92 in ofproto_bundle_register ofproto/ofproto.c:1455 > #8 0x40e6c9 in port_configure vswitchd/bridge.c:1300 > #9 0x40bcfd in bridge_reconfigure vswitchd/bridge.c:921 > #10 0x41f1a9 in bridge_run vswitchd/bridge.c:3313 > #11 0x42d4fb in main vswitchd/ovs-vswitchd.c:132 > #12 0x7f312fbbcc86 in __libc_start_main (/usr/lib64/libc.so.6+0x25c86) > > Fixes: fed8962aff57 ("Add new port VLAN mode "dot1q-tunnel"") > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> Reviewed-by: Simon Horman <simon.horman@corigine.com>
On Mon, Apr 24, 2023 at 02:06:53PM +0200, Ilya Maximets wrote: > On 4/23/23 05:47, wangyunjian wrote: > > Is there any issue with this patch? Why is it in the Superseded state? > > Hi. Not sure what happened here. > I moved it back to 'New' for now. That might have been me conflating things. If so, sorry.
On 4/19/23 08:29, Yunjian Wang wrote: > Currently, bundle->cvlans and xbundle->cvlans are pointing to the > same memory location. This can cause issues if the main thread > modifies bundle->cvlans and frees it while the revalidator thread > is still accessing xbundle->cvlans. This can result in use after > free errors. > > AddressSanitizer: heap-use-after-free on address 0x615000007b08 at pc 0x0000004ede1e bp 0x7f3120ee0310 sp 0x7f3120ee0300 > READ of size 8 at 0x615000007b08 thread T25 (revalidator25) > #0 0x4ede1d in bitmap_is_set lib/bitmap.h:91 > #1 0x4fcb26 in xbundle_allows_cvlan ofproto/ofproto-dpif-xlate.c:2028 > #2 0x4fe279 in input_vid_is_valid ofproto/ofproto-dpif-xlate.c:2294 > #3 0x502abf in xlate_normal ofproto/ofproto-dpif-xlate.c:3051 > #4 0x5164dc in xlate_output_action ofproto/ofproto-dpif-xlate.c:5361 > #5 0x522576 in do_xlate_actions ofproto/ofproto-dpif-xlate.c:7047 > #6 0x52a751 in xlate_actions ofproto/ofproto-dpif-xlate.c:8061 > #7 0x4e2b66 in xlate_key ofproto/ofproto-dpif-upcall.c:2212 > #8 0x4e2e13 in xlate_ukey ofproto/ofproto-dpif-upcall.c:2227 > #9 0x4e345d in revalidate_ukey__ ofproto/ofproto-dpif-upcall.c:2276 > #10 0x4e3f85 in revalidate_ukey ofproto/ofproto-dpif-upcall.c:2395 > #11 0x4e7ac5 in revalidate ofproto/ofproto-dpif-upcall.c:2858 > #12 0x4d9ed3 in udpif_revalidator ofproto/ofproto-dpif-upcall.c:1010 > #13 0x7cd92e in ovsthread_wrapper lib/ovs-thread.c:423 > #14 0x7f312ff01f3a (/usr/lib64/libpthread.so.0+0x8f3a) > #15 0x7f312fc8f51f in clone (/usr/lib64/libc.so.6+0xf851f) > > 0x615000007b08 is located 8 bytes inside of 512-byte region [0x615000007b00,0x615000007d00) > freed by thread T0 here: > #0 0x7f3130378ad8 in free (/usr/lib64/libasan.so.4+0xe0ad8) > #1 0x49044e in bundle_set ofproto/ofproto-dpif.c:3431 > #2 0x444f92 in ofproto_bundle_register ofproto/ofproto.c:1455 > #3 0x40e6c9 in port_configure vswitchd/bridge.c:1300 > #4 0x40bcfd in bridge_reconfigure vswitchd/bridge.c:921 > #5 0x41f1a9 in bridge_run vswitchd/bridge.c:3313 > #6 0x42d4fb in main vswitchd/ovs-vswitchd.c:132 > #7 0x7f312fbbcc86 in __libc_start_main (/usr/lib64/libc.so.6+0x25c86) > > previously allocated by thread T0 here: > #0 0x7f3130378e70 in __interceptor_malloc (/usr/lib64/libasan.so.4+0xe0e70) > #1 0x8757fe in xmalloc__ lib/util.c:140 > #2 0x8758da in xmalloc lib/util.c:175 > #3 0x875927 in xmemdup lib/util.c:188 > #4 0x475f63 in bitmap_clone lib/bitmap.h:79 > #5 0x47797c in vlan_bitmap_clone lib/vlan-bitmap.h:40 > #6 0x49048d in bundle_set ofproto/ofproto-dpif.c:3433 > #7 0x444f92 in ofproto_bundle_register ofproto/ofproto.c:1455 > #8 0x40e6c9 in port_configure vswitchd/bridge.c:1300 > #9 0x40bcfd in bridge_reconfigure vswitchd/bridge.c:921 > #10 0x41f1a9 in bridge_run vswitchd/bridge.c:3313 > #11 0x42d4fb in main vswitchd/ovs-vswitchd.c:132 > #12 0x7f312fbbcc86 in __libc_start_main (/usr/lib64/libc.so.6+0x25c86) > > Fixes: fed8962aff57 ("Add new port VLAN mode "dot1q-tunnel"") > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> > --- > ofproto/ofproto-dpif-xlate.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index c01177718..455ca3dac 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -66,6 +66,7 @@ > #include "tunnel.h" > #include "util.h" > #include "uuid.h" > +#include "vlan-bitmap.h" > > COVERAGE_DEFINE(xlate_actions); > COVERAGE_DEFINE(xlate_actions_oversize); > @@ -1028,7 +1029,12 @@ xlate_xbundle_set(struct xbundle *xbundle, > xbundle->qinq_ethtype = qinq_ethtype; > xbundle->vlan = vlan; > xbundle->trunks = trunks; > - xbundle->cvlans = cvlans; > + if (xbundle->cvlans == NULL) { > + xbundle->cvlans = vlan_bitmap_clone(cvlans); > + } else if (!vlan_bitmap_equal(xbundle->cvlans, cvlans)) { > + ovsrcu_postpone(free, xbundle->cvlans); > + xbundle->cvlans = vlan_bitmap_clone(cvlans); > + } Hi. Is it necessary to postpone the free here? Also, vlan_bitmap_equal() can handle NULL pointers, so we can probably avoid special-casing it. Best regards, Ilya Maximets.
> -----Original Message----- > From: Ilya Maximets [mailto:i.maximets@ovn.org] > Sent: Thursday, May 4, 2023 7:44 PM > To: wangyunjian <wangyunjian@huawei.com>; dev@openvswitch.org > Cc: i.maximets@ovn.org; luyicai <luyicai@huawei.com> > Subject: Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Fix use-after-free when > xlate_actions(). > > On 4/19/23 08:29, Yunjian Wang wrote: > > Currently, bundle->cvlans and xbundle->cvlans are pointing to the same > > memory location. This can cause issues if the main thread modifies > > bundle->cvlans and frees it while the revalidator thread is still > > accessing xbundle->cvlans. This can result in use after free errors. > > > > AddressSanitizer: heap-use-after-free on address 0x615000007b08 at pc > > 0x0000004ede1e bp 0x7f3120ee0310 sp 0x7f3120ee0300 READ of size 8 at > 0x615000007b08 thread T25 (revalidator25) > > #0 0x4ede1d in bitmap_is_set lib/bitmap.h:91 > > #1 0x4fcb26 in xbundle_allows_cvlan ofproto/ofproto-dpif-xlate.c:2028 > > #2 0x4fe279 in input_vid_is_valid ofproto/ofproto-dpif-xlate.c:2294 > > #3 0x502abf in xlate_normal ofproto/ofproto-dpif-xlate.c:3051 > > #4 0x5164dc in xlate_output_action ofproto/ofproto-dpif-xlate.c:5361 > > #5 0x522576 in do_xlate_actions ofproto/ofproto-dpif-xlate.c:7047 > > #6 0x52a751 in xlate_actions ofproto/ofproto-dpif-xlate.c:8061 > > #7 0x4e2b66 in xlate_key ofproto/ofproto-dpif-upcall.c:2212 > > #8 0x4e2e13 in xlate_ukey ofproto/ofproto-dpif-upcall.c:2227 > > #9 0x4e345d in revalidate_ukey__ ofproto/ofproto-dpif-upcall.c:2276 > > #10 0x4e3f85 in revalidate_ukey ofproto/ofproto-dpif-upcall.c:2395 > > #11 0x4e7ac5 in revalidate ofproto/ofproto-dpif-upcall.c:2858 > > #12 0x4d9ed3 in udpif_revalidator ofproto/ofproto-dpif-upcall.c:1010 > > #13 0x7cd92e in ovsthread_wrapper lib/ovs-thread.c:423 > > #14 0x7f312ff01f3a (/usr/lib64/libpthread.so.0+0x8f3a) > > #15 0x7f312fc8f51f in clone (/usr/lib64/libc.so.6+0xf851f) > > > > 0x615000007b08 is located 8 bytes inside of 512-byte region > > [0x615000007b00,0x615000007d00) freed by thread T0 here: > > #0 0x7f3130378ad8 in free (/usr/lib64/libasan.so.4+0xe0ad8) > > #1 0x49044e in bundle_set ofproto/ofproto-dpif.c:3431 > > #2 0x444f92 in ofproto_bundle_register ofproto/ofproto.c:1455 > > #3 0x40e6c9 in port_configure vswitchd/bridge.c:1300 > > #4 0x40bcfd in bridge_reconfigure vswitchd/bridge.c:921 > > #5 0x41f1a9 in bridge_run vswitchd/bridge.c:3313 > > #6 0x42d4fb in main vswitchd/ovs-vswitchd.c:132 > > #7 0x7f312fbbcc86 in __libc_start_main > > (/usr/lib64/libc.so.6+0x25c86) > > > > previously allocated by thread T0 here: > > #0 0x7f3130378e70 in __interceptor_malloc > (/usr/lib64/libasan.so.4+0xe0e70) > > #1 0x8757fe in xmalloc__ lib/util.c:140 > > #2 0x8758da in xmalloc lib/util.c:175 > > #3 0x875927 in xmemdup lib/util.c:188 > > #4 0x475f63 in bitmap_clone lib/bitmap.h:79 > > #5 0x47797c in vlan_bitmap_clone lib/vlan-bitmap.h:40 > > #6 0x49048d in bundle_set ofproto/ofproto-dpif.c:3433 > > #7 0x444f92 in ofproto_bundle_register ofproto/ofproto.c:1455 > > #8 0x40e6c9 in port_configure vswitchd/bridge.c:1300 > > #9 0x40bcfd in bridge_reconfigure vswitchd/bridge.c:921 > > #10 0x41f1a9 in bridge_run vswitchd/bridge.c:3313 > > #11 0x42d4fb in main vswitchd/ovs-vswitchd.c:132 > > #12 0x7f312fbbcc86 in __libc_start_main > > (/usr/lib64/libc.so.6+0x25c86) > > > > Fixes: fed8962aff57 ("Add new port VLAN mode "dot1q-tunnel"") > > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> > > --- > > ofproto/ofproto-dpif-xlate.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/ofproto/ofproto-dpif-xlate.c > > b/ofproto/ofproto-dpif-xlate.c index c01177718..455ca3dac 100644 > > --- a/ofproto/ofproto-dpif-xlate.c > > +++ b/ofproto/ofproto-dpif-xlate.c > > @@ -66,6 +66,7 @@ > > #include "tunnel.h" > > #include "util.h" > > #include "uuid.h" > > +#include "vlan-bitmap.h" > > > > COVERAGE_DEFINE(xlate_actions); > > COVERAGE_DEFINE(xlate_actions_oversize); > > @@ -1028,7 +1029,12 @@ xlate_xbundle_set(struct xbundle *xbundle, > > xbundle->qinq_ethtype = qinq_ethtype; > > xbundle->vlan = vlan; > > xbundle->trunks = trunks; > > - xbundle->cvlans = cvlans; > > + if (xbundle->cvlans == NULL) { > > + xbundle->cvlans = vlan_bitmap_clone(cvlans); > > + } else if (!vlan_bitmap_equal(xbundle->cvlans, cvlans)) { > > + ovsrcu_postpone(free, xbundle->cvlans); > > + xbundle->cvlans = vlan_bitmap_clone(cvlans); > > + } > > Hi. Is it necessary to postpone the free here? Yes, I think it is necessary because the revalidator thread will concurrently access xbundel->cvxlans and requires RCU protection. > Also, vlan_bitmap_equal() can handle NULL pointers, so we can probably avoid > special-casing it. How about this? COVERAGE_DEFINE(xlate_actions_oversize); @@ -1028,7 +1029,12 @@ xlate_xbundle_set(struct xbundle *xbundle, xbundle->qinq_ethtype = qinq_ethtype; xbundle->vlan = vlan; xbundle->trunks = trunks; - xbundle->cvlans = cvlans; + if (!vlan_bitmap_equal(xbundle->cvlans, cvlans)) { + if (xbundle->cvlans) { + ovsrcu_postpone(free, xbundle->cvlans); + } + xbundle->cvlans = vlan_bitmap_clone(cvlans); + } xbundle->use_priority_tags = use_priority_tags; xbundle->floodable = floodable; Thanks, Yunjian > > Best regards, Ilya Maximets.
On 5/4/23 14:20, wangyunjian via dev wrote: > > >> -----Original Message----- >> From: Ilya Maximets [mailto:i.maximets@ovn.org] >> Sent: Thursday, May 4, 2023 7:44 PM >> To: wangyunjian <wangyunjian@huawei.com>; dev@openvswitch.org >> Cc: i.maximets@ovn.org; luyicai <luyicai@huawei.com> >> Subject: Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Fix use-after-free when >> xlate_actions(). >> >> On 4/19/23 08:29, Yunjian Wang wrote: >>> Currently, bundle->cvlans and xbundle->cvlans are pointing to the same >>> memory location. This can cause issues if the main thread modifies >>> bundle->cvlans and frees it while the revalidator thread is still >>> accessing xbundle->cvlans. This can result in use after free errors. >>> >>> AddressSanitizer: heap-use-after-free on address 0x615000007b08 at pc >>> 0x0000004ede1e bp 0x7f3120ee0310 sp 0x7f3120ee0300 READ of size 8 at >> 0x615000007b08 thread T25 (revalidator25) >>> #0 0x4ede1d in bitmap_is_set lib/bitmap.h:91 >>> #1 0x4fcb26 in xbundle_allows_cvlan ofproto/ofproto-dpif-xlate.c:2028 >>> #2 0x4fe279 in input_vid_is_valid ofproto/ofproto-dpif-xlate.c:2294 >>> #3 0x502abf in xlate_normal ofproto/ofproto-dpif-xlate.c:3051 >>> #4 0x5164dc in xlate_output_action ofproto/ofproto-dpif-xlate.c:5361 >>> #5 0x522576 in do_xlate_actions ofproto/ofproto-dpif-xlate.c:7047 >>> #6 0x52a751 in xlate_actions ofproto/ofproto-dpif-xlate.c:8061 >>> #7 0x4e2b66 in xlate_key ofproto/ofproto-dpif-upcall.c:2212 >>> #8 0x4e2e13 in xlate_ukey ofproto/ofproto-dpif-upcall.c:2227 >>> #9 0x4e345d in revalidate_ukey__ ofproto/ofproto-dpif-upcall.c:2276 >>> #10 0x4e3f85 in revalidate_ukey ofproto/ofproto-dpif-upcall.c:2395 >>> #11 0x4e7ac5 in revalidate ofproto/ofproto-dpif-upcall.c:2858 >>> #12 0x4d9ed3 in udpif_revalidator ofproto/ofproto-dpif-upcall.c:1010 >>> #13 0x7cd92e in ovsthread_wrapper lib/ovs-thread.c:423 >>> #14 0x7f312ff01f3a (/usr/lib64/libpthread.so.0+0x8f3a) >>> #15 0x7f312fc8f51f in clone (/usr/lib64/libc.so.6+0xf851f) >>> >>> 0x615000007b08 is located 8 bytes inside of 512-byte region >>> [0x615000007b00,0x615000007d00) freed by thread T0 here: >>> #0 0x7f3130378ad8 in free (/usr/lib64/libasan.so.4+0xe0ad8) >>> #1 0x49044e in bundle_set ofproto/ofproto-dpif.c:3431 >>> #2 0x444f92 in ofproto_bundle_register ofproto/ofproto.c:1455 >>> #3 0x40e6c9 in port_configure vswitchd/bridge.c:1300 >>> #4 0x40bcfd in bridge_reconfigure vswitchd/bridge.c:921 >>> #5 0x41f1a9 in bridge_run vswitchd/bridge.c:3313 >>> #6 0x42d4fb in main vswitchd/ovs-vswitchd.c:132 >>> #7 0x7f312fbbcc86 in __libc_start_main >>> (/usr/lib64/libc.so.6+0x25c86) >>> >>> previously allocated by thread T0 here: >>> #0 0x7f3130378e70 in __interceptor_malloc >> (/usr/lib64/libasan.so.4+0xe0e70) >>> #1 0x8757fe in xmalloc__ lib/util.c:140 >>> #2 0x8758da in xmalloc lib/util.c:175 >>> #3 0x875927 in xmemdup lib/util.c:188 >>> #4 0x475f63 in bitmap_clone lib/bitmap.h:79 >>> #5 0x47797c in vlan_bitmap_clone lib/vlan-bitmap.h:40 >>> #6 0x49048d in bundle_set ofproto/ofproto-dpif.c:3433 >>> #7 0x444f92 in ofproto_bundle_register ofproto/ofproto.c:1455 >>> #8 0x40e6c9 in port_configure vswitchd/bridge.c:1300 >>> #9 0x40bcfd in bridge_reconfigure vswitchd/bridge.c:921 >>> #10 0x41f1a9 in bridge_run vswitchd/bridge.c:3313 >>> #11 0x42d4fb in main vswitchd/ovs-vswitchd.c:132 >>> #12 0x7f312fbbcc86 in __libc_start_main >>> (/usr/lib64/libc.so.6+0x25c86) >>> >>> Fixes: fed8962aff57 ("Add new port VLAN mode "dot1q-tunnel"") >>> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> >>> --- >>> ofproto/ofproto-dpif-xlate.c | 9 ++++++++- >>> 1 file changed, 8 insertions(+), 1 deletion(-) >>> >>> diff --git a/ofproto/ofproto-dpif-xlate.c >>> b/ofproto/ofproto-dpif-xlate.c index c01177718..455ca3dac 100644 >>> --- a/ofproto/ofproto-dpif-xlate.c >>> +++ b/ofproto/ofproto-dpif-xlate.c >>> @@ -66,6 +66,7 @@ >>> #include "tunnel.h" >>> #include "util.h" >>> #include "uuid.h" >>> +#include "vlan-bitmap.h" >>> >>> COVERAGE_DEFINE(xlate_actions); >>> COVERAGE_DEFINE(xlate_actions_oversize); >>> @@ -1028,7 +1029,12 @@ xlate_xbundle_set(struct xbundle *xbundle, >>> xbundle->qinq_ethtype = qinq_ethtype; >>> xbundle->vlan = vlan; >>> xbundle->trunks = trunks; >>> - xbundle->cvlans = cvlans; >>> + if (xbundle->cvlans == NULL) { >>> + xbundle->cvlans = vlan_bitmap_clone(cvlans); >>> + } else if (!vlan_bitmap_equal(xbundle->cvlans, cvlans)) { >>> + ovsrcu_postpone(free, xbundle->cvlans); >>> + xbundle->cvlans = vlan_bitmap_clone(cvlans); >>> + } >> >> Hi. Is it necessary to postpone the free here? > > Yes, I think it is necessary because the revalidator thread will concurrently access > xbundel->cvxlans and requires RCU protection. But wouldn't it have its own copy of xbundle? I mean, we do not protect xbundle->name, for example. And the whole xcfg is RCU-protected. > >> Also, vlan_bitmap_equal() can handle NULL pointers, so we can probably avoid >> special-casing it. > > How about this? > > COVERAGE_DEFINE(xlate_actions_oversize); > @@ -1028,7 +1029,12 @@ xlate_xbundle_set(struct xbundle *xbundle, > xbundle->qinq_ethtype = qinq_ethtype; > xbundle->vlan = vlan; > xbundle->trunks = trunks; > - xbundle->cvlans = cvlans; > + if (!vlan_bitmap_equal(xbundle->cvlans, cvlans)) { > + if (xbundle->cvlans) { > + ovsrcu_postpone(free, xbundle->cvlans); > + } free(NULL) is a valid operation, so no need to check. > + xbundle->cvlans = vlan_bitmap_clone(cvlans); > + } > xbundle->use_priority_tags = use_priority_tags; > xbundle->floodable = floodable; > > Thanks, Yunjian >> >> Best regards, Ilya Maximets. > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
> -----Original Message----- > From: Ilya Maximets [mailto:i.maximets@ovn.org] > Sent: Thursday, May 4, 2023 8:32 PM > To: wangyunjian <wangyunjian@huawei.com>; dev@openvswitch.org > Cc: i.maximets@ovn.org; luyicai <luyicai@huawei.com> > Subject: Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Fix use-after-free when > xlate_actions(). > > On 5/4/23 14:20, wangyunjian via dev wrote: > > > > > >> -----Original Message----- > >> From: Ilya Maximets [mailto:i.maximets@ovn.org] > >> Sent: Thursday, May 4, 2023 7:44 PM > >> To: wangyunjian <wangyunjian@huawei.com>; dev@openvswitch.org > >> Cc: i.maximets@ovn.org; luyicai <luyicai@huawei.com> > >> Subject: Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Fix use-after-free > >> when xlate_actions(). > >> > >> On 4/19/23 08:29, Yunjian Wang wrote: > >>> Currently, bundle->cvlans and xbundle->cvlans are pointing to the > >>> same memory location. This can cause issues if the main thread > >>> modifies > >>> bundle->cvlans and frees it while the revalidator thread is still > >>> accessing xbundle->cvlans. This can result in use after free errors. > >>> > >>> AddressSanitizer: heap-use-after-free on address 0x615000007b08 at > >>> pc 0x0000004ede1e bp 0x7f3120ee0310 sp 0x7f3120ee0300 READ of size 8 > >>> at > >> 0x615000007b08 thread T25 (revalidator25) > >>> #0 0x4ede1d in bitmap_is_set lib/bitmap.h:91 > >>> #1 0x4fcb26 in xbundle_allows_cvlan > ofproto/ofproto-dpif-xlate.c:2028 > >>> #2 0x4fe279 in input_vid_is_valid ofproto/ofproto-dpif-xlate.c:2294 > >>> #3 0x502abf in xlate_normal ofproto/ofproto-dpif-xlate.c:3051 > >>> #4 0x5164dc in xlate_output_action ofproto/ofproto-dpif-xlate.c:5361 > >>> #5 0x522576 in do_xlate_actions ofproto/ofproto-dpif-xlate.c:7047 > >>> #6 0x52a751 in xlate_actions ofproto/ofproto-dpif-xlate.c:8061 > >>> #7 0x4e2b66 in xlate_key ofproto/ofproto-dpif-upcall.c:2212 > >>> #8 0x4e2e13 in xlate_ukey ofproto/ofproto-dpif-upcall.c:2227 > >>> #9 0x4e345d in revalidate_ukey__ ofproto/ofproto-dpif-upcall.c:2276 > >>> #10 0x4e3f85 in revalidate_ukey ofproto/ofproto-dpif-upcall.c:2395 > >>> #11 0x4e7ac5 in revalidate ofproto/ofproto-dpif-upcall.c:2858 > >>> #12 0x4d9ed3 in udpif_revalidator ofproto/ofproto-dpif-upcall.c:1010 > >>> #13 0x7cd92e in ovsthread_wrapper lib/ovs-thread.c:423 > >>> #14 0x7f312ff01f3a (/usr/lib64/libpthread.so.0+0x8f3a) > >>> #15 0x7f312fc8f51f in clone (/usr/lib64/libc.so.6+0xf851f) > >>> > >>> 0x615000007b08 is located 8 bytes inside of 512-byte region > >>> [0x615000007b00,0x615000007d00) freed by thread T0 here: > >>> #0 0x7f3130378ad8 in free (/usr/lib64/libasan.so.4+0xe0ad8) > >>> #1 0x49044e in bundle_set ofproto/ofproto-dpif.c:3431 > >>> #2 0x444f92 in ofproto_bundle_register ofproto/ofproto.c:1455 > >>> #3 0x40e6c9 in port_configure vswitchd/bridge.c:1300 > >>> #4 0x40bcfd in bridge_reconfigure vswitchd/bridge.c:921 > >>> #5 0x41f1a9 in bridge_run vswitchd/bridge.c:3313 > >>> #6 0x42d4fb in main vswitchd/ovs-vswitchd.c:132 > >>> #7 0x7f312fbbcc86 in __libc_start_main > >>> (/usr/lib64/libc.so.6+0x25c86) > >>> > >>> previously allocated by thread T0 here: > >>> #0 0x7f3130378e70 in __interceptor_malloc > >> (/usr/lib64/libasan.so.4+0xe0e70) > >>> #1 0x8757fe in xmalloc__ lib/util.c:140 > >>> #2 0x8758da in xmalloc lib/util.c:175 > >>> #3 0x875927 in xmemdup lib/util.c:188 > >>> #4 0x475f63 in bitmap_clone lib/bitmap.h:79 > >>> #5 0x47797c in vlan_bitmap_clone lib/vlan-bitmap.h:40 > >>> #6 0x49048d in bundle_set ofproto/ofproto-dpif.c:3433 > >>> #7 0x444f92 in ofproto_bundle_register ofproto/ofproto.c:1455 > >>> #8 0x40e6c9 in port_configure vswitchd/bridge.c:1300 > >>> #9 0x40bcfd in bridge_reconfigure vswitchd/bridge.c:921 > >>> #10 0x41f1a9 in bridge_run vswitchd/bridge.c:3313 > >>> #11 0x42d4fb in main vswitchd/ovs-vswitchd.c:132 > >>> #12 0x7f312fbbcc86 in __libc_start_main > >>> (/usr/lib64/libc.so.6+0x25c86) > >>> > >>> Fixes: fed8962aff57 ("Add new port VLAN mode "dot1q-tunnel"") > >>> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> > >>> --- > >>> ofproto/ofproto-dpif-xlate.c | 9 ++++++++- > >>> 1 file changed, 8 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/ofproto/ofproto-dpif-xlate.c > >>> b/ofproto/ofproto-dpif-xlate.c index c01177718..455ca3dac 100644 > >>> --- a/ofproto/ofproto-dpif-xlate.c > >>> +++ b/ofproto/ofproto-dpif-xlate.c > >>> @@ -66,6 +66,7 @@ > >>> #include "tunnel.h" > >>> #include "util.h" > >>> #include "uuid.h" > >>> +#include "vlan-bitmap.h" > >>> > >>> COVERAGE_DEFINE(xlate_actions); > >>> COVERAGE_DEFINE(xlate_actions_oversize); > >>> @@ -1028,7 +1029,12 @@ xlate_xbundle_set(struct xbundle *xbundle, > >>> xbundle->qinq_ethtype = qinq_ethtype; > >>> xbundle->vlan = vlan; > >>> xbundle->trunks = trunks; > >>> - xbundle->cvlans = cvlans; > >>> + if (xbundle->cvlans == NULL) { > >>> + xbundle->cvlans = vlan_bitmap_clone(cvlans); > >>> + } else if (!vlan_bitmap_equal(xbundle->cvlans, cvlans)) { > >>> + ovsrcu_postpone(free, xbundle->cvlans); > >>> + xbundle->cvlans = vlan_bitmap_clone(cvlans); > >>> + } > >> > >> Hi. Is it necessary to postpone the free here? > > > > Yes, I think it is necessary because the revalidator thread will > > concurrently access > > xbundel->cvxlans and requires RCU protection. > > But wouldn't it have its own copy of xbundle? > I mean, we do not protect xbundle->name, for example. > And the whole xcfg is RCU-protected. The problematic call is not within the xcfg's RCU protection, as follows: bridge_run()->bridge_reconfigure()-> port_configure()-> bundle_set(). This is using new xcfg at this time. I think there is also a problem with xbundle->name. Currently xbundle->name is only accessed when trace is enabled, so the problem has not been discovered yet. > > > > >> Also, vlan_bitmap_equal() can handle NULL pointers, so we can > >> probably avoid special-casing it. > > > > How about this? > > > > COVERAGE_DEFINE(xlate_actions_oversize); > > @@ -1028,7 +1029,12 @@ xlate_xbundle_set(struct xbundle *xbundle, > > xbundle->qinq_ethtype = qinq_ethtype; > > xbundle->vlan = vlan; > > xbundle->trunks = trunks; > > - xbundle->cvlans = cvlans; > > + if (!vlan_bitmap_equal(xbundle->cvlans, cvlans)) { > > + if (xbundle->cvlans) { > > + ovsrcu_postpone(free, xbundle->cvlans); > > + } > > free(NULL) is a valid operation, so no need to check. OK > > > + xbundle->cvlans = vlan_bitmap_clone(cvlans); > > + } > > xbundle->use_priority_tags = use_priority_tags; > > xbundle->floodable = floodable; > > > > Thanks, Yunjian > >> > >> Best regards, Ilya Maximets. > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
On 5/5/23 03:46, wangyunjian wrote: > > >> -----Original Message----- >> From: Ilya Maximets [mailto:i.maximets@ovn.org] >> Sent: Thursday, May 4, 2023 8:32 PM >> To: wangyunjian <wangyunjian@huawei.com>; dev@openvswitch.org >> Cc: i.maximets@ovn.org; luyicai <luyicai@huawei.com> >> Subject: Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Fix use-after-free when >> xlate_actions(). >> >> On 5/4/23 14:20, wangyunjian via dev wrote: >>> >>> >>>> -----Original Message----- >>>> From: Ilya Maximets [mailto:i.maximets@ovn.org] >>>> Sent: Thursday, May 4, 2023 7:44 PM >>>> To: wangyunjian <wangyunjian@huawei.com>; dev@openvswitch.org >>>> Cc: i.maximets@ovn.org; luyicai <luyicai@huawei.com> >>>> Subject: Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Fix use-after-free >>>> when xlate_actions(). >>>> >>>> On 4/19/23 08:29, Yunjian Wang wrote: >>>>> Currently, bundle->cvlans and xbundle->cvlans are pointing to the >>>>> same memory location. This can cause issues if the main thread >>>>> modifies >>>>> bundle->cvlans and frees it while the revalidator thread is still >>>>> accessing xbundle->cvlans. This can result in use after free errors. >>>>> >>>>> AddressSanitizer: heap-use-after-free on address 0x615000007b08 at >>>>> pc 0x0000004ede1e bp 0x7f3120ee0310 sp 0x7f3120ee0300 READ of size 8 >>>>> at >>>> 0x615000007b08 thread T25 (revalidator25) >>>>> #0 0x4ede1d in bitmap_is_set lib/bitmap.h:91 >>>>> #1 0x4fcb26 in xbundle_allows_cvlan >> ofproto/ofproto-dpif-xlate.c:2028 >>>>> #2 0x4fe279 in input_vid_is_valid ofproto/ofproto-dpif-xlate.c:2294 >>>>> #3 0x502abf in xlate_normal ofproto/ofproto-dpif-xlate.c:3051 >>>>> #4 0x5164dc in xlate_output_action ofproto/ofproto-dpif-xlate.c:5361 >>>>> #5 0x522576 in do_xlate_actions ofproto/ofproto-dpif-xlate.c:7047 >>>>> #6 0x52a751 in xlate_actions ofproto/ofproto-dpif-xlate.c:8061 >>>>> #7 0x4e2b66 in xlate_key ofproto/ofproto-dpif-upcall.c:2212 >>>>> #8 0x4e2e13 in xlate_ukey ofproto/ofproto-dpif-upcall.c:2227 >>>>> #9 0x4e345d in revalidate_ukey__ ofproto/ofproto-dpif-upcall.c:2276 >>>>> #10 0x4e3f85 in revalidate_ukey ofproto/ofproto-dpif-upcall.c:2395 >>>>> #11 0x4e7ac5 in revalidate ofproto/ofproto-dpif-upcall.c:2858 >>>>> #12 0x4d9ed3 in udpif_revalidator ofproto/ofproto-dpif-upcall.c:1010 >>>>> #13 0x7cd92e in ovsthread_wrapper lib/ovs-thread.c:423 >>>>> #14 0x7f312ff01f3a (/usr/lib64/libpthread.so.0+0x8f3a) >>>>> #15 0x7f312fc8f51f in clone (/usr/lib64/libc.so.6+0xf851f) >>>>> >>>>> 0x615000007b08 is located 8 bytes inside of 512-byte region >>>>> [0x615000007b00,0x615000007d00) freed by thread T0 here: >>>>> #0 0x7f3130378ad8 in free (/usr/lib64/libasan.so.4+0xe0ad8) >>>>> #1 0x49044e in bundle_set ofproto/ofproto-dpif.c:3431 >>>>> #2 0x444f92 in ofproto_bundle_register ofproto/ofproto.c:1455 >>>>> #3 0x40e6c9 in port_configure vswitchd/bridge.c:1300 >>>>> #4 0x40bcfd in bridge_reconfigure vswitchd/bridge.c:921 >>>>> #5 0x41f1a9 in bridge_run vswitchd/bridge.c:3313 >>>>> #6 0x42d4fb in main vswitchd/ovs-vswitchd.c:132 >>>>> #7 0x7f312fbbcc86 in __libc_start_main >>>>> (/usr/lib64/libc.so.6+0x25c86) >>>>> >>>>> previously allocated by thread T0 here: >>>>> #0 0x7f3130378e70 in __interceptor_malloc >>>> (/usr/lib64/libasan.so.4+0xe0e70) >>>>> #1 0x8757fe in xmalloc__ lib/util.c:140 >>>>> #2 0x8758da in xmalloc lib/util.c:175 >>>>> #3 0x875927 in xmemdup lib/util.c:188 >>>>> #4 0x475f63 in bitmap_clone lib/bitmap.h:79 >>>>> #5 0x47797c in vlan_bitmap_clone lib/vlan-bitmap.h:40 >>>>> #6 0x49048d in bundle_set ofproto/ofproto-dpif.c:3433 >>>>> #7 0x444f92 in ofproto_bundle_register ofproto/ofproto.c:1455 >>>>> #8 0x40e6c9 in port_configure vswitchd/bridge.c:1300 >>>>> #9 0x40bcfd in bridge_reconfigure vswitchd/bridge.c:921 >>>>> #10 0x41f1a9 in bridge_run vswitchd/bridge.c:3313 >>>>> #11 0x42d4fb in main vswitchd/ovs-vswitchd.c:132 >>>>> #12 0x7f312fbbcc86 in __libc_start_main >>>>> (/usr/lib64/libc.so.6+0x25c86) >>>>> >>>>> Fixes: fed8962aff57 ("Add new port VLAN mode "dot1q-tunnel"") >>>>> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> >>>>> --- >>>>> ofproto/ofproto-dpif-xlate.c | 9 ++++++++- >>>>> 1 file changed, 8 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/ofproto/ofproto-dpif-xlate.c >>>>> b/ofproto/ofproto-dpif-xlate.c index c01177718..455ca3dac 100644 >>>>> --- a/ofproto/ofproto-dpif-xlate.c >>>>> +++ b/ofproto/ofproto-dpif-xlate.c >>>>> @@ -66,6 +66,7 @@ >>>>> #include "tunnel.h" >>>>> #include "util.h" >>>>> #include "uuid.h" >>>>> +#include "vlan-bitmap.h" >>>>> >>>>> COVERAGE_DEFINE(xlate_actions); >>>>> COVERAGE_DEFINE(xlate_actions_oversize); >>>>> @@ -1028,7 +1029,12 @@ xlate_xbundle_set(struct xbundle *xbundle, >>>>> xbundle->qinq_ethtype = qinq_ethtype; >>>>> xbundle->vlan = vlan; >>>>> xbundle->trunks = trunks; >>>>> - xbundle->cvlans = cvlans; >>>>> + if (xbundle->cvlans == NULL) { >>>>> + xbundle->cvlans = vlan_bitmap_clone(cvlans); >>>>> + } else if (!vlan_bitmap_equal(xbundle->cvlans, cvlans)) { >>>>> + ovsrcu_postpone(free, xbundle->cvlans); >>>>> + xbundle->cvlans = vlan_bitmap_clone(cvlans); >>>>> + } >>>> >>>> Hi. Is it necessary to postpone the free here? >>> >>> Yes, I think it is necessary because the revalidator thread will >>> concurrently access >>> xbundel->cvxlans and requires RCU protection. >> >> But wouldn't it have its own copy of xbundle? >> I mean, we do not protect xbundle->name, for example. >> And the whole xcfg is RCU-protected. > > The problematic call is not within the xcfg's RCU protection, as follows: > bridge_run()->bridge_reconfigure()-> port_configure()-> bundle_set(). But bundle_set() frees the original ofbundle->cvlans, not the copy from the xbundle created in this patch. > This is using new xcfg at this time. > I think there is also a problem with xbundle->name. Currently xbundle->name > is only accessed when trace is enabled, so the problem has not been discovered yet. > >> >>> >>>> Also, vlan_bitmap_equal() can handle NULL pointers, so we can >>>> probably avoid special-casing it. >>> >>> How about this? >>> >>> COVERAGE_DEFINE(xlate_actions_oversize); >>> @@ -1028,7 +1029,12 @@ xlate_xbundle_set(struct xbundle *xbundle, >>> xbundle->qinq_ethtype = qinq_ethtype; >>> xbundle->vlan = vlan; >>> xbundle->trunks = trunks; >>> - xbundle->cvlans = cvlans; >>> + if (!vlan_bitmap_equal(xbundle->cvlans, cvlans)) { >>> + if (xbundle->cvlans) { >>> + ovsrcu_postpone(free, xbundle->cvlans); >>> + } >> >> free(NULL) is a valid operation, so no need to check. > > OK > >> >>> + xbundle->cvlans = vlan_bitmap_clone(cvlans); >>> + } >>> xbundle->use_priority_tags = use_priority_tags; >>> xbundle->floodable = floodable; >>> >>> Thanks, Yunjian >>>> >>>> Best regards, Ilya Maximets. >>> >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >> >
> -----Original Message----- > From: Ilya Maximets [mailto:i.maximets@ovn.org] > Sent: Saturday, May 6, 2023 12:15 AM > To: wangyunjian <wangyunjian@huawei.com>; dev@openvswitch.org > Cc: i.maximets@ovn.org; luyicai <luyicai@huawei.com> > Subject: Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Fix use-after-free when > xlate_actions(). > > On 5/5/23 03:46, wangyunjian wrote: > > > > > >> -----Original Message----- > >> From: Ilya Maximets [mailto:i.maximets@ovn.org] > >> Sent: Thursday, May 4, 2023 8:32 PM > >> To: wangyunjian <wangyunjian@huawei.com>; dev@openvswitch.org > >> Cc: i.maximets@ovn.org; luyicai <luyicai@huawei.com> > >> Subject: Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Fix use-after-free > >> when xlate_actions(). > >> > >> On 5/4/23 14:20, wangyunjian via dev wrote: > >>> > >>> > >>>> -----Original Message----- > >>>> From: Ilya Maximets [mailto:i.maximets@ovn.org] > >>>> Sent: Thursday, May 4, 2023 7:44 PM > >>>> To: wangyunjian <wangyunjian@huawei.com>; dev@openvswitch.org > >>>> Cc: i.maximets@ovn.org; luyicai <luyicai@huawei.com> > >>>> Subject: Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Fix > >>>> use-after-free when xlate_actions(). > >>>> > >>>> On 4/19/23 08:29, Yunjian Wang wrote: > >>>>> Currently, bundle->cvlans and xbundle->cvlans are pointing to the > >>>>> same memory location. This can cause issues if the main thread > >>>>> modifies > >>>>> bundle->cvlans and frees it while the revalidator thread is still > >>>>> accessing xbundle->cvlans. This can result in use after free errors. > >>>>> > >>>>> AddressSanitizer: heap-use-after-free on address 0x615000007b08 at > >>>>> pc 0x0000004ede1e bp 0x7f3120ee0310 sp 0x7f3120ee0300 READ of > size > >>>>> 8 at > >>>> 0x615000007b08 thread T25 (revalidator25) > >>>>> #0 0x4ede1d in bitmap_is_set lib/bitmap.h:91 > >>>>> #1 0x4fcb26 in xbundle_allows_cvlan > >> ofproto/ofproto-dpif-xlate.c:2028 > >>>>> #2 0x4fe279 in input_vid_is_valid ofproto/ofproto-dpif-xlate.c:2294 > >>>>> #3 0x502abf in xlate_normal ofproto/ofproto-dpif-xlate.c:3051 > >>>>> #4 0x5164dc in xlate_output_action > ofproto/ofproto-dpif-xlate.c:5361 > >>>>> #5 0x522576 in do_xlate_actions ofproto/ofproto-dpif-xlate.c:7047 > >>>>> #6 0x52a751 in xlate_actions ofproto/ofproto-dpif-xlate.c:8061 > >>>>> #7 0x4e2b66 in xlate_key ofproto/ofproto-dpif-upcall.c:2212 > >>>>> #8 0x4e2e13 in xlate_ukey ofproto/ofproto-dpif-upcall.c:2227 > >>>>> #9 0x4e345d in revalidate_ukey__ > ofproto/ofproto-dpif-upcall.c:2276 > >>>>> #10 0x4e3f85 in revalidate_ukey ofproto/ofproto-dpif-upcall.c:2395 > >>>>> #11 0x4e7ac5 in revalidate ofproto/ofproto-dpif-upcall.c:2858 > >>>>> #12 0x4d9ed3 in udpif_revalidator > ofproto/ofproto-dpif-upcall.c:1010 > >>>>> #13 0x7cd92e in ovsthread_wrapper lib/ovs-thread.c:423 > >>>>> #14 0x7f312ff01f3a (/usr/lib64/libpthread.so.0+0x8f3a) > >>>>> #15 0x7f312fc8f51f in clone (/usr/lib64/libc.so.6+0xf851f) > >>>>> > >>>>> 0x615000007b08 is located 8 bytes inside of 512-byte region > >>>>> [0x615000007b00,0x615000007d00) freed by thread T0 here: > >>>>> #0 0x7f3130378ad8 in free (/usr/lib64/libasan.so.4+0xe0ad8) > >>>>> #1 0x49044e in bundle_set ofproto/ofproto-dpif.c:3431 > >>>>> #2 0x444f92 in ofproto_bundle_register ofproto/ofproto.c:1455 > >>>>> #3 0x40e6c9 in port_configure vswitchd/bridge.c:1300 > >>>>> #4 0x40bcfd in bridge_reconfigure vswitchd/bridge.c:921 > >>>>> #5 0x41f1a9 in bridge_run vswitchd/bridge.c:3313 > >>>>> #6 0x42d4fb in main vswitchd/ovs-vswitchd.c:132 > >>>>> #7 0x7f312fbbcc86 in __libc_start_main > >>>>> (/usr/lib64/libc.so.6+0x25c86) > >>>>> > >>>>> previously allocated by thread T0 here: > >>>>> #0 0x7f3130378e70 in __interceptor_malloc > >>>> (/usr/lib64/libasan.so.4+0xe0e70) > >>>>> #1 0x8757fe in xmalloc__ lib/util.c:140 > >>>>> #2 0x8758da in xmalloc lib/util.c:175 > >>>>> #3 0x875927 in xmemdup lib/util.c:188 > >>>>> #4 0x475f63 in bitmap_clone lib/bitmap.h:79 > >>>>> #5 0x47797c in vlan_bitmap_clone lib/vlan-bitmap.h:40 > >>>>> #6 0x49048d in bundle_set ofproto/ofproto-dpif.c:3433 > >>>>> #7 0x444f92 in ofproto_bundle_register ofproto/ofproto.c:1455 > >>>>> #8 0x40e6c9 in port_configure vswitchd/bridge.c:1300 > >>>>> #9 0x40bcfd in bridge_reconfigure vswitchd/bridge.c:921 > >>>>> #10 0x41f1a9 in bridge_run vswitchd/bridge.c:3313 > >>>>> #11 0x42d4fb in main vswitchd/ovs-vswitchd.c:132 > >>>>> #12 0x7f312fbbcc86 in __libc_start_main > >>>>> (/usr/lib64/libc.so.6+0x25c86) > >>>>> > >>>>> Fixes: fed8962aff57 ("Add new port VLAN mode "dot1q-tunnel"") > >>>>> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> > >>>>> --- > >>>>> ofproto/ofproto-dpif-xlate.c | 9 ++++++++- > >>>>> 1 file changed, 8 insertions(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/ofproto/ofproto-dpif-xlate.c > >>>>> b/ofproto/ofproto-dpif-xlate.c index c01177718..455ca3dac 100644 > >>>>> --- a/ofproto/ofproto-dpif-xlate.c > >>>>> +++ b/ofproto/ofproto-dpif-xlate.c > >>>>> @@ -66,6 +66,7 @@ > >>>>> #include "tunnel.h" > >>>>> #include "util.h" > >>>>> #include "uuid.h" > >>>>> +#include "vlan-bitmap.h" > >>>>> > >>>>> COVERAGE_DEFINE(xlate_actions); > >>>>> COVERAGE_DEFINE(xlate_actions_oversize); > >>>>> @@ -1028,7 +1029,12 @@ xlate_xbundle_set(struct xbundle *xbundle, > >>>>> xbundle->qinq_ethtype = qinq_ethtype; > >>>>> xbundle->vlan = vlan; > >>>>> xbundle->trunks = trunks; > >>>>> - xbundle->cvlans = cvlans; > >>>>> + if (xbundle->cvlans == NULL) { > >>>>> + xbundle->cvlans = vlan_bitmap_clone(cvlans); > >>>>> + } else if (!vlan_bitmap_equal(xbundle->cvlans, cvlans)) { > >>>>> + ovsrcu_postpone(free, xbundle->cvlans); > >>>>> + xbundle->cvlans = vlan_bitmap_clone(cvlans); > >>>>> + } > >>>> > >>>> Hi. Is it necessary to postpone the free here? > >>> > >>> Yes, I think it is necessary because the revalidator thread will > >>> concurrently access > >>> xbundel->cvxlans and requires RCU protection. > >> > >> But wouldn't it have its own copy of xbundle? > >> I mean, we do not protect xbundle->name, for example. > >> And the whole xcfg is RCU-protected. > > > > The problematic call is not within the xcfg's RCU protection, as follows: > > bridge_run()->bridge_reconfigure()-> port_configure()-> bundle_set(). > > But bundle_set() frees the original ofbundle->cvlans, not the copy from the > xbundle created in this patch. Thank you for your careful guidance. I understand and agree with you. I send a new patch. http://patchwork.ozlabs.org/project/openvswitch/patch/1683367209-7320-1-git-send-email-wangyunjian@huawei.com/ > > > This is using new xcfg at this time. > > I think there is also a problem with xbundle->name. Currently > > xbundle->name is only accessed when trace is enabled, so the problem has > not been discovered yet. > > > >> > >>> > >>>> Also, vlan_bitmap_equal() can handle NULL pointers, so we can > >>>> probably avoid special-casing it. > >>> > >>> How about this? > >>> > >>> COVERAGE_DEFINE(xlate_actions_oversize); > >>> @@ -1028,7 +1029,12 @@ xlate_xbundle_set(struct xbundle *xbundle, > >>> xbundle->qinq_ethtype = qinq_ethtype; > >>> xbundle->vlan = vlan; > >>> xbundle->trunks = trunks; > >>> - xbundle->cvlans = cvlans; > >>> + if (!vlan_bitmap_equal(xbundle->cvlans, cvlans)) { > >>> + if (xbundle->cvlans) { > >>> + ovsrcu_postpone(free, xbundle->cvlans); > >>> + } > >> > >> free(NULL) is a valid operation, so no need to check. > > > > OK > > > >> > >>> + xbundle->cvlans = vlan_bitmap_clone(cvlans); > >>> + } > >>> xbundle->use_priority_tags = use_priority_tags; > >>> xbundle->floodable = floodable; > >>> > >>> Thanks, Yunjian > >>>> > >>>> Best regards, Ilya Maximets. > >>> > >>> _______________________________________________ > >>> dev mailing list > >>> dev@openvswitch.org > >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >>> > >> > > >
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index c01177718..455ca3dac 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -66,6 +66,7 @@ #include "tunnel.h" #include "util.h" #include "uuid.h" +#include "vlan-bitmap.h" COVERAGE_DEFINE(xlate_actions); COVERAGE_DEFINE(xlate_actions_oversize); @@ -1028,7 +1029,12 @@ xlate_xbundle_set(struct xbundle *xbundle, xbundle->qinq_ethtype = qinq_ethtype; xbundle->vlan = vlan; xbundle->trunks = trunks; - xbundle->cvlans = cvlans; + if (xbundle->cvlans == NULL) { + xbundle->cvlans = vlan_bitmap_clone(cvlans); + } else if (!vlan_bitmap_equal(xbundle->cvlans, cvlans)) { + ovsrcu_postpone(free, xbundle->cvlans); + xbundle->cvlans = vlan_bitmap_clone(cvlans); + } xbundle->use_priority_tags = use_priority_tags; xbundle->floodable = floodable; xbundle->protected = protected; @@ -1380,6 +1386,7 @@ xlate_xbundle_remove(struct xlate_cfg *xcfg, struct xbundle *xbundle) ovs_list_remove(&xbundle->list_node); bond_unref(xbundle->bond); lacp_unref(xbundle->lacp); + free(xbundle->cvlans); free(xbundle->name); free(xbundle); }
Currently, bundle->cvlans and xbundle->cvlans are pointing to the same memory location. This can cause issues if the main thread modifies bundle->cvlans and frees it while the revalidator thread is still accessing xbundle->cvlans. This can result in use after free errors. AddressSanitizer: heap-use-after-free on address 0x615000007b08 at pc 0x0000004ede1e bp 0x7f3120ee0310 sp 0x7f3120ee0300 READ of size 8 at 0x615000007b08 thread T25 (revalidator25) #0 0x4ede1d in bitmap_is_set lib/bitmap.h:91 #1 0x4fcb26 in xbundle_allows_cvlan ofproto/ofproto-dpif-xlate.c:2028 #2 0x4fe279 in input_vid_is_valid ofproto/ofproto-dpif-xlate.c:2294 #3 0x502abf in xlate_normal ofproto/ofproto-dpif-xlate.c:3051 #4 0x5164dc in xlate_output_action ofproto/ofproto-dpif-xlate.c:5361 #5 0x522576 in do_xlate_actions ofproto/ofproto-dpif-xlate.c:7047 #6 0x52a751 in xlate_actions ofproto/ofproto-dpif-xlate.c:8061 #7 0x4e2b66 in xlate_key ofproto/ofproto-dpif-upcall.c:2212 #8 0x4e2e13 in xlate_ukey ofproto/ofproto-dpif-upcall.c:2227 #9 0x4e345d in revalidate_ukey__ ofproto/ofproto-dpif-upcall.c:2276 #10 0x4e3f85 in revalidate_ukey ofproto/ofproto-dpif-upcall.c:2395 #11 0x4e7ac5 in revalidate ofproto/ofproto-dpif-upcall.c:2858 #12 0x4d9ed3 in udpif_revalidator ofproto/ofproto-dpif-upcall.c:1010 #13 0x7cd92e in ovsthread_wrapper lib/ovs-thread.c:423 #14 0x7f312ff01f3a (/usr/lib64/libpthread.so.0+0x8f3a) #15 0x7f312fc8f51f in clone (/usr/lib64/libc.so.6+0xf851f) 0x615000007b08 is located 8 bytes inside of 512-byte region [0x615000007b00,0x615000007d00) freed by thread T0 here: #0 0x7f3130378ad8 in free (/usr/lib64/libasan.so.4+0xe0ad8) #1 0x49044e in bundle_set ofproto/ofproto-dpif.c:3431 #2 0x444f92 in ofproto_bundle_register ofproto/ofproto.c:1455 #3 0x40e6c9 in port_configure vswitchd/bridge.c:1300 #4 0x40bcfd in bridge_reconfigure vswitchd/bridge.c:921 #5 0x41f1a9 in bridge_run vswitchd/bridge.c:3313 #6 0x42d4fb in main vswitchd/ovs-vswitchd.c:132 #7 0x7f312fbbcc86 in __libc_start_main (/usr/lib64/libc.so.6+0x25c86) previously allocated by thread T0 here: #0 0x7f3130378e70 in __interceptor_malloc (/usr/lib64/libasan.so.4+0xe0e70) #1 0x8757fe in xmalloc__ lib/util.c:140 #2 0x8758da in xmalloc lib/util.c:175 #3 0x875927 in xmemdup lib/util.c:188 #4 0x475f63 in bitmap_clone lib/bitmap.h:79 #5 0x47797c in vlan_bitmap_clone lib/vlan-bitmap.h:40 #6 0x49048d in bundle_set ofproto/ofproto-dpif.c:3433 #7 0x444f92 in ofproto_bundle_register ofproto/ofproto.c:1455 #8 0x40e6c9 in port_configure vswitchd/bridge.c:1300 #9 0x40bcfd in bridge_reconfigure vswitchd/bridge.c:921 #10 0x41f1a9 in bridge_run vswitchd/bridge.c:3313 #11 0x42d4fb in main vswitchd/ovs-vswitchd.c:132 #12 0x7f312fbbcc86 in __libc_start_main (/usr/lib64/libc.so.6+0x25c86) Fixes: fed8962aff57 ("Add new port VLAN mode "dot1q-tunnel"") Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> --- ofproto/ofproto-dpif-xlate.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)