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

Flavio Fernandes flavio at flaviof.com
Wed Mar 17 14:12:20 UTC 2021



> On Mar 16, 2021, at 5:28 PM, Ilya Maximets <i.maximets at ovn.org> wrote:
> 
> 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. :)
> 

ha. no worries, and yes to your review feedbacks! will do.

-- flaviof


>> +
>> 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;
>>>>     }
>>>> 
>>>> 
>>> 
>> 
> 



More information about the dev mailing list