On 5/20/21 6:55 PM, Mark Gray wrote: > On 04/04/2021 18:31, Ilya Maximets wrote: >> ct limit requests never initializes the whole 'struct ovs_zone_limit' >> sending uninitialized stack memory to kernel: >> >> Syscall param sendmsg(msg.msg_iov[0]) points to uninitialised byte(s) >> at 0x5E23867: sendmsg (in /usr/lib64/libpthread-2.28.so) >> by 0x54F761: nl_sock_transact_multiple__ (netlink-socket.c:858) >> by 0x54FB6E: nl_sock_transact_multiple.part.9 (netlink-socket.c:1079) >> by 0x54FCC0: nl_sock_transact_multiple (netlink-socket.c:1044) >> by 0x54FCC0: nl_sock_transact (netlink-socket.c:1108) >> by 0x550B6F: nl_transact (netlink-socket.c:1804) >> by 0x53BEA2: dpif_netlink_ct_get_limits (dpif-netlink.c:3052) >> by 0x588B57: dpctl_ct_get_limits (dpctl.c:2178) >> by 0x586FF2: dpctl_unixctl_handler (dpctl.c:2870) >> by 0x52C241: process_command (unixctl.c:310) >> by 0x52C241: run_connection (unixctl.c:344) >> by 0x52C241: unixctl_server_run (unixctl.c:395) >> by 0x407526: main (ovs-vswitchd.c:128) >> Address 0x10b87480 is 32 bytes inside a block of size 4,096 alloc'd >> at 0x4C30F0B: malloc (vg_replace_malloc.c:307) >> by 0x52CDE4: xmalloc (util.c:138) >> by 0x4F7E07: ofpbuf_init (ofpbuf.c:123) >> by 0x4F7E07: ofpbuf_new (ofpbuf.c:151) >> by 0x53BDE3: dpif_netlink_ct_get_limits (dpif-netlink.c:3025) >> by 0x588B57: dpctl_ct_get_limits (dpctl.c:2178) >> by 0x586FF2: dpctl_unixctl_handler (dpctl.c:2870) >> by 0x52C241: process_command (unixctl.c:310) >> by 0x52C241: run_connection (unixctl.c:344) >> by 0x52C241: unixctl_server_run (unixctl.c:395) >> by 0x407526: main (ovs-vswitchd.c:128) >> Uninitialised value was created by a stack allocation >> at 0x46AAA0: ct_dpif_get_limits (ct-dpif.c:197) >> >> Fix that by using designated initializers that will clear all the >> non-specified fields. >> >> CC: Yi-Hung Wei <[email protected]> >> Fixes: 906ff9d229ee ("dpif-netlink: Implement conntrack zone limit") >> Signed-off-by: Ilya Maximets <[email protected]> >> --- >> lib/dpif-netlink.c | 24 ++++++++++++++---------- >> 1 file changed, 14 insertions(+), 10 deletions(-) >> >> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c >> index ceb56c685..ee96f4011 100644 >> --- a/lib/dpif-netlink.c >> +++ b/lib/dpif-netlink.c >> @@ -2923,8 +2923,6 @@ dpif_netlink_ct_set_limits(struct dpif *dpif >> OVS_UNUSED, >> const uint32_t *default_limits, >> const struct ovs_list *zone_limits) >> { >> - struct ovs_zone_limit req_zone_limit; >> - >> if (ovs_ct_limit_family < 0) { >> return EOPNOTSUPP; >> } >> @@ -2941,8 +2939,10 @@ dpif_netlink_ct_set_limits(struct dpif *dpif >> OVS_UNUSED, >> size_t opt_offset; >> opt_offset = nl_msg_start_nested(request, OVS_CT_LIMIT_ATTR_ZONE_LIMIT); >> if (default_limits) { >> - req_zone_limit.zone_id = OVS_ZONE_LIMIT_DEFAULT_ZONE; >> - req_zone_limit.limit = *default_limits; >> + struct ovs_zone_limit req_zone_limit = { >> + .zone_id = OVS_ZONE_LIMIT_DEFAULT_ZONE, >> + .limit = *default_limits, >> + }; >> nl_msg_put(request, &req_zone_limit, sizeof req_zone_limit); >> } >> >> @@ -2950,8 +2950,10 @@ dpif_netlink_ct_set_limits(struct dpif *dpif >> OVS_UNUSED, >> struct ct_dpif_zone_limit *zone_limit; >> >> LIST_FOR_EACH (zone_limit, node, zone_limits) { >> - req_zone_limit.zone_id = zone_limit->zone; >> - req_zone_limit.limit = zone_limit->limit; >> + struct ovs_zone_limit req_zone_limit = { >> + .zone_id = zone_limit->zone, >> + .limit = zone_limit->limit, >> + }; >> nl_msg_put(request, &req_zone_limit, sizeof req_zone_limit); >> } >> } >> @@ -3035,8 +3037,9 @@ dpif_netlink_ct_get_limits(struct dpif *dpif >> OVS_UNUSED, >> size_t opt_offset = nl_msg_start_nested(request, >> >> OVS_CT_LIMIT_ATTR_ZONE_LIMIT); >> >> - struct ovs_zone_limit req_zone_limit; >> - req_zone_limit.zone_id = OVS_ZONE_LIMIT_DEFAULT_ZONE; >> + struct ovs_zone_limit req_zone_limit = { >> + .zone_id = OVS_ZONE_LIMIT_DEFAULT_ZONE, >> + }; >> nl_msg_put(request, &req_zone_limit, sizeof req_zone_limit); >> >> struct ct_dpif_zone_limit *zone_limit; >> @@ -3086,8 +3089,9 @@ dpif_netlink_ct_del_limits(struct dpif *dpif >> OVS_UNUSED, >> >> struct ct_dpif_zone_limit *zone_limit; >> LIST_FOR_EACH (zone_limit, node, zone_limits) { >> - struct ovs_zone_limit req_zone_limit; >> - req_zone_limit.zone_id = zone_limit->zone; >> + struct ovs_zone_limit req_zone_limit = { >> + .zone_id = zone_limit->zone, > > Does this set 'limit' and 'count' automatically to 0? Is this behaviour > specified by the relevant c standard or is it compiler dependent?
Yes. C standard says following: If there are fewer initializers in a list than there are members of an aggregate, the remainder of the aggregate shall be initialized implicitly the same as objects that have static storage duration. This means that they will be zeroed. This doesn't work for initialization of unions with fields that have different sizes, but it's not the case here. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
