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

[Ilya Maximets]

On 3/16/21 10:27 PM, Ilya Maximets wrote:
> On 3/16/21 10:21 PM, Ilya Maximets wrote:
>> 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)
> 
> Oops, I see that report because I modified the test like this:
> 
> diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
> index 3e6222557..244b10ac7 100644
> --- a/tests/dpif-netdev.at
> +++ b/tests/dpif-netdev.at
> @@ -370,6 +370,8 @@ recirc_id(0),in_port(7),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), a
>  recirc_id(0),in_port(8),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), actions:2
>  ])
>  
> +dnl AT_CHECK([ovs-ofctl -O OpenFlow13 del-meters br0])

s/dnl //
Sorry.  I need some rest, apparently. :)

> +
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
>  
> ---
> 
> It doesn't happen on master.
> Flavio, could you, please, also add above test modification
> to the patch?
> 
>>
>>
>> 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;
>>>      }
>>>  
>>>
>>
>