Re: [ovs-dev] [PATCH 1/1] ofp-parse: Fix segfault due to bad meter n_bands
> On Mar 16, 2021, at 5:28 PM, Ilya Maximets 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(, 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
---
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;
}
>>>
>>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/1] ofp-parse: Fix segfault due to bad meter n_bands
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(, 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
>>> ---
>>> 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;
>>> }
>>>
>>>
>>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/1] ofp-parse: Fix segfault due to bad meter n_bands
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(, 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
>> ---
>> 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;
>> }
>>
>>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/1] ofp-parse: Fix segfault due to bad meter n_bands
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(, 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
> ---
> 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;
> }
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 1/1] ofp-parse: Fix segfault due to bad meter n_bands
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
---
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;
}
--
2.25.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
