On Wed, Jul 17, 2019 at 7:55 PM Ben Pfaff <b...@ovn.org> wrote: > > The following, run inside the OVS sandbox, caused OVS to abort when > Address Sanitizer was used: > > ovs-vsctl add-br br-int > ovs-ofctl add-flow br-int > "table=0,cookie=0x1234,priority=10000,icmp,actions=drop" > ovs-ofctl --strict del-flows br-int > "table=0,cookie=0x1234/-1,priority=10000" > > Sample report from Address Sanitizer: > > ==3029==ERROR: AddressSanitizer: heap-buffer-overflow on address > 0x603000043260 at pc 0x7f6b09c2459b bp 0x7ffcb67e7540 sp 0x7ffcb67e6cf0 > READ of size 40 at 0x603000043260 thread T0 > #0 0x7f6b09c2459a (/lib/x86_64-linux-gnu/libasan.so.5+0xb859a) > #1 0x565110a748a5 in minimask_equal ../lib/flow.c:3510 > #2 0x565110a9ea41 in minimatch_equal ../lib/match.c:1821 > #3 0x56511091e864 in collect_rules_strict ../ofproto/ofproto.c:4516 > #4 0x56511093d526 in delete_flow_start_strict ../ofproto/ofproto.c:5959 > #5 0x56511093d526 in ofproto_flow_mod_start ../ofproto/ofproto.c:7949 > #6 0x56511093d77b in handle_flow_mod__ ../ofproto/ofproto.c:6122 > #7 0x56511093db71 in handle_flow_mod ../ofproto/ofproto.c:6099 > #8 0x5651109407f6 in handle_single_part_openflow ../ofproto/ofproto.c:8406 > #9 0x5651109407f6 in handle_openflow ../ofproto/ofproto.c:8587 > #10 0x5651109e40da in ofconn_run ../ofproto/connmgr.c:1318 > #11 0x5651109e40da in connmgr_run ../ofproto/connmgr.c:355 > #12 0x56511092b129 in ofproto_run ../ofproto/ofproto.c:1826 > #13 0x5651108f23cd in bridge_run__ ../vswitchd/bridge.c:2965 > #14 0x565110904887 in bridge_run ../vswitchd/bridge.c:3023 > #15 0x5651108e659c in main ../vswitchd/ovs-vswitchd.c:127 > #16 0x7f6b093b709a in __libc_start_main ../csu/libc-start.c:308 > #17 0x5651108e9009 in _start > (/home/blp/nicira/ovs/_build/vswitchd/ovs-vswitchd+0x11d009) > > This fixes the problem, which although largely theoretical could crop up > with odd implementations of memcmp(), perhaps ones optimized in various > "clever" ways. All in all, it seems best to avoid the theoretical problem. > > Signed-off-by: Ben Pfaff <b...@ovn.org>
Cool! Looks good (and interesting) to me Acked-by: Dumitru Ceara <dce...@redhat.com> > --- > lib/flow.c | 19 ++++++++++++++++--- > 1 file changed, 16 insertions(+), 3 deletions(-) > > diff --git a/lib/flow.c b/lib/flow.c > index 95da7d4b1803..e54fd2e522e6 100644 > --- a/lib/flow.c > +++ b/lib/flow.c > @@ -1,5 +1,5 @@ > /* > - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2017 > Nicira, Inc. > + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2017, 2019 > Nicira, Inc. > * > * Licensed under the Apache License, Version 2.0 (the "License"); > * you may not use this file except in compliance with the License. > @@ -3506,8 +3506,21 @@ minimask_expand(const struct minimask *mask, struct > flow_wildcards *wc) > bool > minimask_equal(const struct minimask *a, const struct minimask *b) > { > - return !memcmp(a, b, sizeof *a > - + MINIFLOW_VALUES_SIZE(miniflow_n_values(&a->masks))); > + /* At first glance, it might seem that this can be reasonably optimized > + * into a single memcmp() for the total size of the region. Such an > + * optimization will work OK with most implementations of memcmp() that > + * proceed from the start of the regions to be compared to the end in > + * reasonably sized chunks. However, memcmp() is not required to be > + * implemented that way, and an implementation that, for example, > compares > + * all of the bytes in both regions without early exit when it finds a > + * difference, or one that compares, say, 64 bytes at a time, could > access > + * an unmapped region of memory if minimasks 'a' and 'b' have different > + * lengths. By first checking that the maps are the same with the first > + * memcmp(), we verify that 'a' and 'b' have the same length and > therefore > + * ensure that the second memcmp() is safe. */ > + return (!memcmp(a, b, sizeof *a) > + && !memcmp(a + 1, b + 1, > + MINIFLOW_VALUES_SIZE(miniflow_n_values(&a->masks)))); > } > > /* Returns true if at least one bit matched by 'b' is wildcarded by 'a', > -- > 2.20.1 > > _______________________________________________ > 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