diff mbox series

[ovs-dev] odp-util: Fix passing uninitialized bytes in OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV*.

Message ID 20200109162736.3716-1-i.maximets@ovn.org
State Accepted
Headers show
Series [ovs-dev] odp-util: Fix passing uninitialized bytes in OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV*. | expand

Commit Message

Ilya Maximets Jan. 9, 2020, 4:27 p.m. UTC 
Both ovs_key_ct_tuple_ipv* structures contains padding at the end
that mast be cleared before passing attributes to kernel:

 Syscall param sendmsg(msg.msg_iov[0]) points to uninitialised byte(s)
    at 0x566A607: sendmsg (sendmsg.c:28)
    by 0xFC95CE: nl_sock_transact_multiple__ (netlink-socket.c:858)
    by 0xFC8580: nl_sock_transact_multiple (netlink-socket.c:1079)
    by 0xFC83FF: nl_transact_multiple (netlink-socket.c:1839)
    by 0xFA8648: dpif_netlink_operate__ (dpif-netlink.c:1926)
    by 0xFA789F: dpif_netlink_operate_chunks (dpif-netlink.c:2219)
    by 0xFA25CB: dpif_netlink_operate (dpif-netlink.c:2278)
    by 0xE5BB4C: dpif_operate (dpif.c:1377)
    by 0xE5B7F6: dpif_flow_put (dpif.c:1048)
    by 0xE5B49A: dpif_probe_feature (dpif.c:965)
    by 0xDD6BF5: check_ct_orig_tuple (ofproto-dpif.c:1557)
    by 0xDD41EC: check_support (ofproto-dpif.c:1590)
    by 0xDD3BF3: open_dpif_backer (ofproto-dpif.c:818)
    by 0xDC8467: construct (ofproto-dpif.c:1605)
    by 0xDAD6BB: ofproto_create (ofproto.c:549)
    by 0xD96A19: bridge_reconfigure (bridge.c:877)
    by 0xD9625D: bridge_run (bridge.c:3324)
    by 0xDA5829: main (ovs-vswitchd.c:127)
  Address 0x1ffefe36a5 is on thread 1's stack
  in frame #4, created by dpif_netlink_operate__ (dpif-netlink.c:1839)
  Uninitialised value was created by a stack allocation
    at 0xEB87D0: odp_flow_key_from_flow__ (odp-util.c:5996)

Fixes: daf4d3c18da4 ("odp: Support conntrack orig tuple key.")
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 lib/odp-util.c | 40 ++++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

Comments

Ben Pfaff Jan. 9, 2020, 6:16 p.m. UTC | #1 
On Thu, Jan 09, 2020 at 05:27:36PM +0100, Ilya Maximets wrote:
> Both ovs_key_ct_tuple_ipv* structures contains padding at the end
> that mast be cleared before passing attributes to kernel:

Good catch.

I think that you could save a line for the memset() call by using
nl_msg_put_unspec_zero().

Acked-by: Ben Pfaff <blp@ovn.org>
Ilya Maximets Jan. 13, 2020, 5:55 p.m. UTC | #2 
On 09.01.2020 19:16, Ben Pfaff wrote:
> On Thu, Jan 09, 2020 at 05:27:36PM +0100, Ilya Maximets wrote:
>> Both ovs_key_ct_tuple_ipv* structures contains padding at the end
>> that mast be cleared before passing attributes to kernel:
> 
> Good catch.
> 
> I think that you could save a line for the memset() call by using
> nl_msg_put_unspec_zero().
> 
> Acked-by: Ben Pfaff <blp@ovn.org>
> 

Thanks!  I updated this patch to use nl_msg_put_unspec_zero().
Applied to master and backported.

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 122572415..105b01a88 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -6032,26 +6032,30 @@  odp_flow_key_from_flow__(const struct odp_flow_key_parms *parms,
     if (flow->ct_nw_proto) {
         if (parms->support.ct_orig_tuple
             && flow->dl_type == htons(ETH_TYPE_IP)) {
-            struct ovs_key_ct_tuple_ipv4 ct = {
-                data->ct_nw_src,
-                data->ct_nw_dst,
-                data->ct_tp_src,
-                data->ct_tp_dst,
-                data->ct_nw_proto,
-            };
-            nl_msg_put_unspec(buf, OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4, &ct,
-                              sizeof ct);
+            struct ovs_key_ct_tuple_ipv4 *ct;
+
+            ct = nl_msg_put_unspec_uninit(buf, OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4,
+                                          sizeof *ct);
+            /* 'struct ovs_key_ct_tuple_ipv4' has padding, clear it. */
+            memset(ct, 0, sizeof *ct);
+            ct->ipv4_src = data->ct_nw_src;
+            ct->ipv4_dst = data->ct_nw_dst;
+            ct->src_port = data->ct_tp_src;
+            ct->dst_port = data->ct_tp_dst;
+            ct->ipv4_proto = data->ct_nw_proto;
         } else if (parms->support.ct_orig_tuple6
                    && flow->dl_type == htons(ETH_TYPE_IPV6)) {
-            struct ovs_key_ct_tuple_ipv6 ct = {
-                data->ct_ipv6_src,
-                data->ct_ipv6_dst,
-                data->ct_tp_src,
-                data->ct_tp_dst,
-                data->ct_nw_proto,
-            };
-            nl_msg_put_unspec(buf, OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6, &ct,
-                              sizeof ct);
+            struct ovs_key_ct_tuple_ipv6 *ct;
+
+            ct = nl_msg_put_unspec_uninit(buf, OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6,
+                                          sizeof *ct);
+            /* 'struct ovs_key_ct_tuple_ipv6' has padding, clear it. */
+            memset(ct, 0, sizeof *ct);
+            ct->ipv6_src = data->ct_ipv6_src;
+            ct->ipv6_dst = data->ct_ipv6_dst;
+            ct->src_port = data->ct_tp_src;
+            ct->dst_port = data->ct_tp_dst;
+            ct->ipv6_proto = data->ct_nw_proto;
         }
     }
     if (parms->support.recirc) {