[ovs-dev] [PATCH 5/5] dpif-netlink: Fix send of uninitialized memory in ct limit requests.
Ilya Maximets
i.maximets at ovn.org
Mon May 24 19:44:15 UTC 2021
On 5/21/21 9:23 AM, Mark Gray wrote:
> On 20/05/2021 19:24, Ilya Maximets wrote:
>> On 5/20/21 7:46 PM, Ilya Maximets wrote:
>>> 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.
>>
>> To be more correct, this also doesn't work well with paddings inside the
>> structure, i.e. doesn't clear them. But this structure has no paddings.
>>
>>>
>>> Best regards, Ilya Maximets.
>>>
>>
>
> Thanks for checking. LGTM
>
> Acked-by: Mark D. Gray <mark.d.gray at redhat.com>
>
Thanks! Applied to master and backported down to 2.12.
Best regards, Ilya Maximets.
More information about the dev
mailing list