| Message ID | 20210317181418.1446094-2-flavio@flaviof.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | ofp-parse: Fix segfault due to bad meter n_bands | expand |
Flavio Fernandes <flavio@flaviof.com> writes: > 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. > > Once del-meters change in test dpif-netdev.at is added, the valgrind report on test > '992: dpif-netdev - meters' shows this issue: > > 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) > > Fixes: 3200ed5805 ("ovs-ofctl: Add meter support.") > Signed-off-by: Flavio Fernandes <flavio@flaviof.com> > --- > v3: > - Nit: Fix commit message > > v2: > - Use memset to initialize struct instead of setting individial members > - Invoke del-meters in existing tests/dpif-netdev.at test > --- Nice, and thanks for including the test case enhancement :) Acked-by: Aaron Conole <aconole@redhat.com>
On 3/17/21 8:40 PM, Aaron Conole wrote: > Flavio Fernandes <flavio@flaviof.com> writes: > >> 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. >> >> Once del-meters change in test dpif-netdev.at is added, the valgrind report on test >> '992: dpif-netdev - meters' shows this issue: >> >> 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) >> >> Fixes: 3200ed5805 ("ovs-ofctl: Add meter support.") >> Signed-off-by: Flavio Fernandes <flavio@flaviof.com> >> --- >> v3: >> - Nit: Fix commit message >> >> v2: >> - Use memset to initialize struct instead of setting individial members >> - Invoke del-meters in existing tests/dpif-netdev.at test >> --- > > Nice, and thanks for including the test case enhancement :) > > Acked-by: Aaron Conole <aconole@redhat.com> Thanks! Applied to master and backported down to 2.12. Best regards, Ilya Maximets.
diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at index 3e6222557..e223446d4 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 ]) +AT_CHECK([ovs-ofctl -O OpenFlow13 del-meters br0]) + OVS_VSWITCHD_STOP AT_CLEANUP diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c index 3601890f4..62059e962 100644 --- a/utilities/ovs-ofctl.c +++ b/utilities/ovs-ofctl.c @@ -4020,6 +4020,7 @@ ofctl_meter_mod__(const char *bridge, const char *str, int command) enum ofputil_protocol usable_protocols; enum ofp_version version; + memset(&mm, 0, sizeof mm); if (str) { char *error; error = parse_ofp_meter_mod_str(&mm, str, command, &usable_protocols); @@ -4030,7 +4031,6 @@ 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.bands = NULL; } protocol = open_vconn_for_flow_mod(bridge, &vconn, usable_protocols); @@ -4050,6 +4050,7 @@ ofctl_meter_request__(const char *bridge, const char *str, enum ofputil_protocol protocol; enum ofp_version version; + memset(&mm, 0, sizeof mm); if (str) { char *error; error = parse_ofp_meter_mod_str(&mm, str, -1, &usable_protocols); @@ -4059,7 +4060,6 @@ ofctl_meter_request__(const char *bridge, const char *str, } else { usable_protocols = OFPUTIL_P_OF13_UP; mm.meter.meter_id = OFPM13_ALL; - mm.meter.bands = NULL; } protocol = open_vconn_for_flow_mod(bridge, &vconn, usable_protocols);
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. Once del-meters change in test dpif-netdev.at is added, the valgrind report on test '992: dpif-netdev - meters' shows this issue: 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) Fixes: 3200ed5805 ("ovs-ofctl: Add meter support.") Signed-off-by: Flavio Fernandes <flavio@flaviof.com> --- v3: - Nit: Fix commit message v2: - Use memset to initialize struct instead of setting individial members - Invoke del-meters in existing tests/dpif-netdev.at test --- tests/dpif-netdev.at | 2 ++ utilities/ovs-ofctl.c | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-)