| Message ID | 20190725163104.14297-1-i.maximets@samsung.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ofproto-dpif: Fix using uninitialised memory in user_action_cookie. | expand |
On 25.07.2019 19:31, Ilya Maximets wrote: > 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> > --- Kind reminder. It'll be good to have this before the official 2.12 release. Best regards, Ilya Maximets. > ofproto/ofproto-dpif-upcall.c | 1 + > ofproto/ofproto-dpif-xlate.c | 52 +++++++++++++++++++---------------- > 2 files changed, 29 insertions(+), 24 deletions(-) > > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index 731ee7280..f46cdf213 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -1064,6 +1064,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.ofp_in_port = ofp_in_port; > cookie.ofproto_uuid = *ofproto_uuid; > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 28a7fdd84..b7da8cead 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -3214,11 +3214,13 @@ compose_sflow_action(struct xlate_ctx *ctx) > return 0; > } > > - struct user_action_cookie cookie = { > - .type = USER_ACTION_COOKIE_SFLOW, > - .ofp_in_port = ctx->xin->flow.in_port.ofp_port, > - .ofproto_uuid = ctx->xbridge->ofproto->uuid > - }; > + struct user_action_cookie cookie; > + > + memset(&cookie, 0, sizeof cookie); > + cookie.type = USER_ACTION_COOKIE_SFLOW; > + cookie.ofp_in_port = ctx->xin->flow.in_port.ofp_port; > + cookie.ofproto_uuid = ctx->xbridge->ofproto->uuid; > + > return compose_sample_action(ctx, dpif_sflow_get_probability(sflow), > &cookie, ODPP_NONE, true); > } > @@ -3258,12 +3260,14 @@ compose_ipfix_action(struct xlate_ctx *ctx, odp_port_t output_odp_port) > } > } > > - struct user_action_cookie cookie = { > - .type = USER_ACTION_COOKIE_IPFIX, > - .ofp_in_port = ctx->xin->flow.in_port.ofp_port, > - .ofproto_uuid = ctx->xbridge->ofproto->uuid, > - .ipfix.output_odp_port = output_odp_port > - }; > + struct user_action_cookie cookie; > + > + memset(&cookie, 0, sizeof cookie); > + cookie.type = USER_ACTION_COOKIE_IPFIX; > + cookie.ofp_in_port = ctx->xin->flow.in_port.ofp_port; > + cookie.ofproto_uuid = ctx->xbridge->ofproto->uuid; > + cookie.ipfix.output_odp_port = output_odp_port; > + > compose_sample_action(ctx, > dpif_ipfix_get_bridge_exporter_probability(ipfix), > &cookie, tunnel_out_port, false); > @@ -5521,19 +5525,19 @@ xlate_sample_action(struct xlate_ctx *ctx, > } > } > > - struct user_action_cookie cookie = { > - .type = USER_ACTION_COOKIE_FLOW_SAMPLE, > - .ofp_in_port = ctx->xin->flow.in_port.ofp_port, > - .ofproto_uuid = ctx->xbridge->ofproto->uuid, > - .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, > - .output_odp_port = output_odp_port, > - .direction = os->direction, > - } > - }; > + struct user_action_cookie cookie; > + > + memset(&cookie, 0, sizeof cookie); > + cookie.type = USER_ACTION_COOKIE_FLOW_SAMPLE; > + cookie.ofp_in_port = ctx->xin->flow.in_port.ofp_port; > + cookie.ofproto_uuid = ctx->xbridge->ofproto->uuid; > + 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; > + cookie.flow_sample.output_odp_port = output_odp_port; > + cookie.flow_sample.direction = os->direction; > + > compose_sample_action(ctx, probability, &cookie, tunnel_out_port, false); > } > >
On Thu, Jul 25, 2019 at 07:31:04PM +0300, Ilya Maximets wrote: > 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(). Acked-by: Ben Pfaff <blp@ovn.org>
> On Thu, Jul 25, 2019 at 07:31:04PM +0300, Ilya Maximets wrote: >> 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(). > > Acked-by: Ben Pfaff <blp@ovn.org> Thanks! Applied to master and backported. Best regards, Ilya Maximets.
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 731ee7280..f46cdf213 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -1064,6 +1064,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.ofp_in_port = ofp_in_port; cookie.ofproto_uuid = *ofproto_uuid; diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 28a7fdd84..b7da8cead 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -3214,11 +3214,13 @@ compose_sflow_action(struct xlate_ctx *ctx) return 0; } - struct user_action_cookie cookie = { - .type = USER_ACTION_COOKIE_SFLOW, - .ofp_in_port = ctx->xin->flow.in_port.ofp_port, - .ofproto_uuid = ctx->xbridge->ofproto->uuid - }; + struct user_action_cookie cookie; + + memset(&cookie, 0, sizeof cookie); + cookie.type = USER_ACTION_COOKIE_SFLOW; + cookie.ofp_in_port = ctx->xin->flow.in_port.ofp_port; + cookie.ofproto_uuid = ctx->xbridge->ofproto->uuid; + return compose_sample_action(ctx, dpif_sflow_get_probability(sflow), &cookie, ODPP_NONE, true); } @@ -3258,12 +3260,14 @@ compose_ipfix_action(struct xlate_ctx *ctx, odp_port_t output_odp_port) } } - struct user_action_cookie cookie = { - .type = USER_ACTION_COOKIE_IPFIX, - .ofp_in_port = ctx->xin->flow.in_port.ofp_port, - .ofproto_uuid = ctx->xbridge->ofproto->uuid, - .ipfix.output_odp_port = output_odp_port - }; + struct user_action_cookie cookie; + + memset(&cookie, 0, sizeof cookie); + cookie.type = USER_ACTION_COOKIE_IPFIX; + cookie.ofp_in_port = ctx->xin->flow.in_port.ofp_port; + cookie.ofproto_uuid = ctx->xbridge->ofproto->uuid; + cookie.ipfix.output_odp_port = output_odp_port; + compose_sample_action(ctx, dpif_ipfix_get_bridge_exporter_probability(ipfix), &cookie, tunnel_out_port, false); @@ -5521,19 +5525,19 @@ xlate_sample_action(struct xlate_ctx *ctx, } } - struct user_action_cookie cookie = { - .type = USER_ACTION_COOKIE_FLOW_SAMPLE, - .ofp_in_port = ctx->xin->flow.in_port.ofp_port, - .ofproto_uuid = ctx->xbridge->ofproto->uuid, - .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, - .output_odp_port = output_odp_port, - .direction = os->direction, - } - }; + struct user_action_cookie cookie; + + memset(&cookie, 0, sizeof cookie); + cookie.type = USER_ACTION_COOKIE_FLOW_SAMPLE; + cookie.ofp_in_port = ctx->xin->flow.in_port.ofp_port; + cookie.ofproto_uuid = ctx->xbridge->ofproto->uuid; + 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; + cookie.flow_sample.output_odp_port = output_odp_port; + cookie.flow_sample.direction = os->direction; + compose_sample_action(ctx, probability, &cookie, tunnel_out_port, false); }
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> --- ofproto/ofproto-dpif-upcall.c | 1 + ofproto/ofproto-dpif-xlate.c | 52 +++++++++++++++++++---------------- 2 files changed, 29 insertions(+), 24 deletions(-)