[ovs-dev] [PATCH] flow: Avoid unsafe comparison of minimasks.

[Dumitru Ceara]

On Wed, Jul 17, 2019 at 7:55 PM Ben Pfaff <blp at 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 at ovn.org>

Cool! Looks good (and interesting) to me

Acked-by: Dumitru Ceara <dceara at 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 at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev