Skip to content

Commit

Permalink
ofproto-dpif: Fix using uninitialised memory in user_action_cookie.
Browse files Browse the repository at this point in the history
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 8de6ff3 ("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.

Change-Id: I6895b1a8ed41f84cb3124e1b5932c0baee26bdea
CC: Justin Pettit <jpettit@ovn.org>
Fixes: 8b7ea2d ("Extend OVS IPFIX exporter to export tunnel headers")
Fixes: 8de6ff3 ("ofproto-dpif: Use a fixed size userspace cookie.")
Fixes: fcb9579 ("ofproto: Add 'ofproto_uuid' and 'ofp_in_port' to user action cookie.")
Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
  • Loading branch information
igsilya committed Jul 25, 2019
1 parent d560bc1 commit ac35a16
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 24 deletions.
1 change: 1 addition & 0 deletions ofproto/ofproto-dpif-upcall.c
Expand Up @@ -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;
Expand Down
52 changes: 28 additions & 24 deletions ofproto/ofproto-dpif-xlate.c
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}

Expand Down

0 comments on commit ac35a16

Please sign in to comment.