| Message ID | 20210404173146.3834196-5-i.maximets@ovn.org |
|---|---|
| State | Rejected |
| Headers | show |
| Series | Fix use of uninitialized memory in various places. | expand |
On 04/04/2021 18:31, Ilya Maximets wrote: > 'cdtp' is allocated on a stack and it has uninitialized 'present' > field that specifies which attributes are actually set. This > causes use of uninitialized attributes. > > Conditional jump or move depends on uninitialised value(s) > at 0x539651: dpif_netlink_get_nl_tp_udp_attrs (dpif-netlink.c:3206) > by 0x539651: dpif_netlink_get_nl_tp_attrs (dpif-netlink.c:3234) > by 0x539651: dpif_netlink_ct_set_timeout_policy (dpif-netlink.c:3370) > by 0x42B615: ct_add_timeout_policy_to_dpif (ofproto-dpif.c:5421) > by 0x42B615: ct_set_zone_timeout_policy (ofproto-dpif.c:5525) > by 0x40BD98: ct_zones_reconfigure (bridge.c:756) > by 0x40BD98: datapath_reconfigure (bridge.c:804) > by 0x40D737: bridge_reconfigure (bridge.c:903) > by 0x411720: bridge_run (bridge.c:3331) > by 0x40751C: main (ovs-vswitchd.c:127) > > Clearing the whole structure to avoid this kind of problems. > > CC: Yi-Hung Wei <yihung.wei@gmail.com> > Fixes: 993cae678bca ("ofproto-dpif: Consume CT_Zone, and CT_Timeout_Policy tables") > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- > ofproto/ofproto-dpif.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index fd0b2fdea..47db9bb57 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -5413,6 +5413,8 @@ ct_add_timeout_policy_to_dpif(struct dpif *dpif, > struct ct_dpif_timeout_policy cdtp; > struct simap_node *node; > > + memset(&cdtp, 0, sizeof cdtp); > + > cdtp.id = ct_tp->tp_id; > SIMAP_FOR_EACH (node, &ct_tp->tp) { > ct_dpif_set_timeout_policy_attr_by_name(&cdtp, node->name, node->data); > LGTM Acked-by: Mark D. Gray <mark.d.gray@redhat.com>
On 5/20/21 6:49 PM, Mark Gray wrote: > On 04/04/2021 18:31, Ilya Maximets wrote: >> 'cdtp' is allocated on a stack and it has uninitialized 'present' >> field that specifies which attributes are actually set. This >> causes use of uninitialized attributes. >> >> Conditional jump or move depends on uninitialised value(s) >> at 0x539651: dpif_netlink_get_nl_tp_udp_attrs (dpif-netlink.c:3206) >> by 0x539651: dpif_netlink_get_nl_tp_attrs (dpif-netlink.c:3234) >> by 0x539651: dpif_netlink_ct_set_timeout_policy (dpif-netlink.c:3370) >> by 0x42B615: ct_add_timeout_policy_to_dpif (ofproto-dpif.c:5421) >> by 0x42B615: ct_set_zone_timeout_policy (ofproto-dpif.c:5525) >> by 0x40BD98: ct_zones_reconfigure (bridge.c:756) >> by 0x40BD98: datapath_reconfigure (bridge.c:804) >> by 0x40D737: bridge_reconfigure (bridge.c:903) >> by 0x411720: bridge_run (bridge.c:3331) >> by 0x40751C: main (ovs-vswitchd.c:127) >> >> Clearing the whole structure to avoid this kind of problems. >> >> CC: Yi-Hung Wei <yihung.wei@gmail.com> >> Fixes: 993cae678bca ("ofproto-dpif: Consume CT_Zone, and CT_Timeout_Policy tables") >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >> --- >> ofproto/ofproto-dpif.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >> index fd0b2fdea..47db9bb57 100644 >> --- a/ofproto/ofproto-dpif.c >> +++ b/ofproto/ofproto-dpif.c >> @@ -5413,6 +5413,8 @@ ct_add_timeout_policy_to_dpif(struct dpif *dpif, >> struct ct_dpif_timeout_policy cdtp; >> struct simap_node *node; >> >> + memset(&cdtp, 0, sizeof cdtp); >> + >> cdtp.id = ct_tp->tp_id; >> SIMAP_FOR_EACH (node, &ct_tp->tp) { >> ct_dpif_set_timeout_policy_attr_by_name(&cdtp, node->name, node->data); >> > LGTM > > Acked-by: Mark D. Gray <mark.d.gray@redhat.com> > Thanks! Applied to master and backported down to 2.13. Best regards, Ilya Maximets.
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index fd0b2fdea..47db9bb57 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -5413,6 +5413,8 @@ ct_add_timeout_policy_to_dpif(struct dpif *dpif, struct ct_dpif_timeout_policy cdtp; struct simap_node *node; + memset(&cdtp, 0, sizeof cdtp); + cdtp.id = ct_tp->tp_id; SIMAP_FOR_EACH (node, &ct_tp->tp) { ct_dpif_set_timeout_policy_attr_by_name(&cdtp, node->name, node->data);
'cdtp' is allocated on a stack and it has uninitialized 'present' field that specifies which attributes are actually set. This causes use of uninitialized attributes. Conditional jump or move depends on uninitialised value(s) at 0x539651: dpif_netlink_get_nl_tp_udp_attrs (dpif-netlink.c:3206) by 0x539651: dpif_netlink_get_nl_tp_attrs (dpif-netlink.c:3234) by 0x539651: dpif_netlink_ct_set_timeout_policy (dpif-netlink.c:3370) by 0x42B615: ct_add_timeout_policy_to_dpif (ofproto-dpif.c:5421) by 0x42B615: ct_set_zone_timeout_policy (ofproto-dpif.c:5525) by 0x40BD98: ct_zones_reconfigure (bridge.c:756) by 0x40BD98: datapath_reconfigure (bridge.c:804) by 0x40D737: bridge_reconfigure (bridge.c:903) by 0x411720: bridge_run (bridge.c:3331) by 0x40751C: main (ovs-vswitchd.c:127) Clearing the whole structure to avoid this kind of problems. CC: Yi-Hung Wei <yihung.wei@gmail.com> Fixes: 993cae678bca ("ofproto-dpif: Consume CT_Zone, and CT_Timeout_Policy tables") Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- ofproto/ofproto-dpif.c | 2 ++ 1 file changed, 2 insertions(+)