diff options
author | Ilya Maximets <i.maximets@samsung.com> | 2019-07-25 18:11:13 +0300 |
---|---|---|
committer | Ilya Maximets <i.maximets@ovn.org> | 2019-08-24 00:17:06 +0300 |
commit | c1d9818506fd63b37f820bd2d881ae0a26bab7fd (patch) | |
tree | 7d562a30f2574d2a0e9fea71cbb184cc9601d0e4 | |
parent | 034212f57e436d0f7895bd25e8515557890eca9e (diff) | |
download | openvswitch-c1d9818506fd63b37f820bd2d881ae0a26bab7fd.tar.gz |
ofproto-dpif: Fix using uninitialised memory in user_action_cookie.
Designated initializers are not suitable for initializing non-packed
structures and unions which are subjects for comparison by memcmp().
Whole memory for 'struct user_action_cookie' must be explicitly cleared
before using because it will be copied with memcpy and later compared
by memcmp in ofpbuf_equal().
Few issues found be valgrind:
Thread 13 revalidator11:
Conditional jump or move depends on uninitialised value(s)
at 0x4C35D96: __memcmp_sse4_1 (in vgpreload_memcheck.so)
by 0x9D4404: ofpbuf_equal (ofpbuf.h:273)
by 0x9D4404: revalidate_ukey__ (ofproto-dpif-upcall.c:2219)
by 0x9D4404: revalidate_ukey (ofproto-dpif-upcall.c:2286)
by 0x9D62AC: revalidate (ofproto-dpif-upcall.c:2685)
by 0x9D62AC: udpif_revalidator (ofproto-dpif-upcall.c:942)
by 0xA9C732: ovsthread_wrapper (ovs-thread.c:383)
by 0x5FF86DA: start_thread (pthread_create.c:463)
by 0x6AF488E: clone (clone.S:95)
Uninitialised value was created by a stack allocation
at 0x9D4450: compose_slow_path (ofproto-dpif-upcall.c:1062)
Thread 11 revalidator16:
Conditional jump or move depends on uninitialised value(s)
at 0x4C35D96: __memcmp_sse4_1 (in vgpreload_memcheck.so)
by 0x9D4404: ofpbuf_equal (ofpbuf.h:273)
by 0x9D4404: revalidate_ukey__ (ofproto-dpif-upcall.c:2220)
by 0x9D4404: revalidate_ukey (ofproto-dpif-upcall.c:2287)
by 0x9D62BC: revalidate (ofproto-dpif-upcall.c:2686)
by 0x9D62BC: udpif_revalidator (ofproto-dpif-upcall.c:942)
by 0xA9C6D2: ovsthread_wrapper (ovs-thread.c:383)
by 0x5FF86DA: start_thread (pthread_create.c:463)
by 0x6AF488E: clone (clone.S:95)
Uninitialised value was created by a stack allocation
at 0x9DC4E0: compose_sflow_action (ofproto-dpif-xlate.c:3211)
The struct was never marked as 'packed', however it was manually
adjusted to be so in practice.
Old IPFIX related commit first made the structure non-contiguous.
Commit 8de6ff3ea864 ("ofproto-dpif: Use a fixed size userspace cookie.")
added uninitialized parts of the additional union space and the next
one introduced new holes between structure fields for all cases.
CC: Justin Pettit <jpettit@ovn.org>
Fixes: 8b7ea2d48033 ("Extend OVS IPFIX exporter to export tunnel headers")
Fixes: 8de6ff3ea864 ("ofproto-dpif: Use a fixed size userspace cookie.")
Fixes: fcb9579be3c7 ("ofproto: Add 'ofproto_uuid' and 'ofp_in_port' to user action cookie.")
Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
Acked-by: Ben Pfaff <blp@ovn.org>
-rw-r--r-- | ofproto/ofproto-dpif-upcall.c | 1 | ||||
-rw-r--r-- | ofproto/ofproto-dpif-xlate.c | 36 |
2 files changed, 21 insertions, 16 deletions
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index eb95dea3c..39fc4a656 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -973,6 +973,7 @@ compose_slow_path(struct udpif *udpif, struct xlate_out *xout, odp_port_t port; uint32_t pid; + memset(&cookie, 0, sizeof cookie); cookie.type = USER_ACTION_COOKIE_SLOW_PATH; cookie.slow_path.unused = 0; cookie.slow_path.reason = xout->slow; diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 8e9b07c67..ab172b94f 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -2585,7 +2585,11 @@ compose_sflow_action(struct xlate_ctx *ctx) return 0; } - union user_action_cookie cookie = { .type = USER_ACTION_COOKIE_SFLOW }; + union user_action_cookie cookie; + + memset(&cookie, 0, sizeof cookie); + cookie.type = USER_ACTION_COOKIE_SFLOW; + return compose_sample_action(ctx, dpif_sflow_get_probability(sflow), &cookie, sizeof cookie.sflow, ODPP_NONE, true); @@ -2624,12 +2628,12 @@ compose_ipfix_action(struct xlate_ctx *ctx, odp_port_t output_odp_port) } } - union user_action_cookie cookie = { - .ipfix = { - .type = USER_ACTION_COOKIE_IPFIX, - .output_odp_port = output_odp_port, - } - }; + union user_action_cookie cookie; + + memset(&cookie, 0, sizeof cookie); + cookie.ipfix.type = USER_ACTION_COOKIE_IPFIX; + cookie.ipfix.output_odp_port = output_odp_port; + compose_sample_action(ctx, dpif_ipfix_get_bridge_exporter_probability(ipfix), &cookie, sizeof cookie.ipfix, tunnel_out_port, @@ -4098,15 +4102,15 @@ xlate_sample_action(struct xlate_ctx *ctx, xlate_commit_actions(ctx); - union user_action_cookie cookie = { - .flow_sample = { - .type = USER_ACTION_COOKIE_FLOW_SAMPLE, - .probability = os->probability, - .collector_set_id = os->collector_set_id, - .obs_domain_id = os->obs_domain_id, - .obs_point_id = os->obs_point_id, - } - }; + union user_action_cookie cookie; + + memset(&cookie, 0, sizeof cookie); + cookie.flow_sample.type = USER_ACTION_COOKIE_FLOW_SAMPLE; + cookie.flow_sample.probability = os->probability; + cookie.flow_sample.collector_set_id = os->collector_set_id; + cookie.flow_sample.obs_domain_id = os->obs_domain_id; + cookie.flow_sample.obs_point_id = os->obs_point_id; + compose_sample_action(ctx, probability, &cookie, sizeof cookie.flow_sample, ODPP_NONE, false); } |