diff mbox series

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

Message ID 20210316202417.1394539-2-flavio@flaviof.com
State Superseded
Headers show
Series ofp-parse: Fix segfault due to bad meter n_bands | expand

Commit Message

Flaviof March 16, 2021, 8:24 p.m. UTC 
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.

Fixes: 3200ed5805 ("ovs-ofctl: Add meter support.")
Signed-off-by: Flavio Fernandes <flavio@flaviof.com>
---
 utilities/ovs-ofctl.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Ilya Maximets March 16, 2021, 9:21 p.m. UTC | #1 
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@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;
>      }
>  
>
Ilya Maximets March 16, 2021, 9:27 p.m. UTC | #2 
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])
+
 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@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;
>>      }
>>  
>>
>
Ilya Maximets March 16, 2021, 9:28 p.m. UTC | #3 
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@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;
>>>      }
>>>  
>>>
>>
>
Flaviof March 17, 2021, 2:12 p.m. UTC | #4 
> On Mar 16, 2021, at 5:28 PM, Ilya Maximets <i.maximets@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@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;
>>>>     }
>>>> 
>>>> 
>>> 
>> 
>
diff mbox series

Patch

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