[ovs-dev] [PATCH 5/5] dpif-netlink: Fix send of uninitialized memory in ct limit requests.

[Ilya Maximets]

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 <yihung.wei at gmail.com>
>> Fixes: 906ff9d229ee ("dpif-netlink: Implement conntrack zone limit")
>> Signed-off-by: Ilya Maximets <i.maximets at ovn.org>
>> ---
>>  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.