[ovs-dev] [PATCH 1/1] ofp-parse: Fix segfault due to bad meter n_bands

Ilya Maximets i.maximets at ovn.org
Tue Mar 16 21:21:58 UTC 2021


On 3/16/21 9:24 PM, Flavio Fernandes wrote:
> Meter commands internally use ofctl_meter_mod__ and ofctl_meter_request__ functions,
> which have an optional parameter called str. When str is NULL, these 2 functions initialize
> a struct with meter bands set as NULL. It also needs to set meter n_bands to 0.

Good catch!  Indeed, I can see the issue from the valgrind report
on test '992: dpif-netdev - meters':

   Conditional jump or move depends on uninitialised value(s)
      at 0x473534: ofputil_put_bands (ofp-meter.c:207)
      by 0x473534: ofputil_encode_meter_mod (ofp-meter.c:557)
      by 0x40FBA2: ofctl_meter_mod__ (ovs-ofctl.c:4038)
      by 0x417BD3: ovs_cmdl_run_command__ (command-line.c:247)
      by 0x4078BA: main (ovs-ofctl.c:179)
    Uninitialised value was created by a stack allocation
      at 0x409350: ofctl_del_meters (ovs-ofctl.c:4088)


For the implementation, I think we need to just memset(&mm, 0, sizeof mm);
the whole structure to zero before the 'if' condition and not initialize
anything that doesn't have a value, i.e. remove setting of
mm.meter.bands to NULL.

Could you update the patch this way?  It might be also good if you can
include above valgrind log into commit message.

Best regards, Ilya Maximets.

> 
> Fixes: 3200ed5805 ("ovs-ofctl: Add meter support.")
> Signed-off-by: Flavio Fernandes <flavio at flaviof.com>
> ---
>  utilities/ovs-ofctl.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index 3601890f4..bd1ba9222 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -4030,6 +4030,8 @@ ofctl_meter_mod__(const char *bridge, const char *str, int command)
>          usable_protocols = OFPUTIL_P_OF13_UP;
>          mm.command = command;
>          mm.meter.meter_id = OFPM13_ALL;
> +        mm.meter.flags = 0;
> +        mm.meter.n_bands = 0;
>          mm.meter.bands = NULL;
>      }
>  
> @@ -4059,6 +4061,8 @@ ofctl_meter_request__(const char *bridge, const char *str,
>      } else {
>          usable_protocols = OFPUTIL_P_OF13_UP;
>          mm.meter.meter_id = OFPM13_ALL;
> +        mm.meter.flags = 0;
> +        mm.meter.n_bands = 0;
>          mm.meter.bands = NULL;
>      }
>  
> 



More information about the dev mailing list