| Message ID | 20190717175516.3179-1-blp@ovn.org |
|---|---|
| State | Accepted |
| Commit | 2ed6505555cdcb46f9b1f0329d1491b75290fc73 |
| Headers | show |
| Series | [ovs-dev] flow: Avoid unsafe comparison of minimasks. | expand |
Bleep bloop. Greetings Ben Pfaff, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Line is 88 characters long (recommended limit is 79) #52 FILE: lib/flow.c:2: * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2017, 2019 Nicira, Inc. Lines checked: 82, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email aconole@bytheb.org Thanks, 0-day Robot
On Wed, Jul 17, 2019 at 7:55 PM Ben Pfaff <blp@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 <blp@ovn.org> Cool! Looks good (and interesting) to me Acked-by: Dumitru Ceara <dceara@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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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',
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 <blp@ovn.org> --- lib/flow.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)