> -----Original Message-----
> From: Ilya Maximets [mailto:i.maxim...@ovn.org]
> Sent: Saturday, May 6, 2023 12:15 AM
> To: wangyunjian <wangyunj...@huawei.com>; d...@openvswitch.org
> Cc: i.maxim...@ovn.org; luyicai <luyi...@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.maxim...@ovn.org]
> >> Sent: Thursday, May 4, 2023 8:32 PM
> >> To: wangyunjian <wangyunj...@huawei.com>; d...@openvswitch.org
> >> Cc: i.maxim...@ovn.org; luyicai <luyi...@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.maxim...@ovn.org]
> >>>> Sent: Thursday, May 4, 2023 7:44 PM
> >>>> To: wangyunjian <wangyunj...@huawei.com>; d...@openvswitch.org
> >>>> Cc: i.maxim...@ovn.org; luyicai <luyi...@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 <wangyunj...@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-wangyunj...@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
> >>> d...@openvswitch.org
> >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >>>
> >>
> >
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
