Daniel Ding <[email protected]> writes:
> From: Daniel Ding <[email protected]>
>
> In order to collect meters since the last meter action
> to the current action in the dpif_execute_helper_cb,
> the next entry is simply obtained using nl_attr_next, and
> without considering the nested nl. This will cause memory
> access to be overflowed.
>
> Gdb debug looks like this:
>
> (gdb) print a
> $6 = (const struct nlattr *) 0xaaaad175a69c
> (gdb) n
> 1206 a = nl_attr_next(a);
> (gdb) n
> 1207 } while (a != action &&
> (gdb) n
> 1206 a = nl_attr_next(a);
> (gdb) print a
> $7 = (const struct nlattr *) 0xaaaad175a69c
> (gdb) print *a
> $8 = {nla_len = 0, nla_type = 0}
> (gdb)
>
> Fixes: 076caa2fb077 ("ofproto: Meter translation.")
> Signed-off-by: Daniel Ding <[email protected]>
> ---
> lib/dpif.c | 43 ++++++++++++++++---------------------------
> 1 file changed, 16 insertions(+), 27 deletions(-)
>
> diff --git a/lib/dpif.c b/lib/dpif.c
> index d07241f1e..05dfd2919 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -1158,11 +1158,14 @@ dpif_flow_dump_next(struct dpif_flow_dump_thread
> *thread,
> return n;
> }
>
> +enum { EXECUTE_HELPER_MAX_METER = 32 };
> +
Could we also prevent this with something like:
while (a != action && nl_attr_is_valid(a) &&
nl_attr_type(a) != OVS_ACTION_ATTR_METER)
I haven't done a deep review, just wondering because from a quick
glance, it seems that this change technically imposes limits on the
action list we are helping with, and then silently drops them after
that.
Also, seems you could include a test case with the fix.
> struct dpif_execute_helper_aux {
> struct dpif *dpif;
> const struct flow *flow;
> int error;
> - const struct nlattr *meter_action; /* Non-NULL, if have a meter action.
> */
> + size_t meter_count;
> + const struct nlattr *meter_actions[EXECUTE_HELPER_MAX_METER + 1];
> };
>
> /* This is called for actions that need the context of the datapath to be
> @@ -1179,9 +1182,9 @@ dpif_execute_helper_cb(void *aux_, struct
> dp_packet_batch *packets_,
>
> switch ((enum ovs_action_attr)type) {
> case OVS_ACTION_ATTR_METER:
> - /* Maintain a pointer to the first meter action seen. */
> - if (!aux->meter_action) {
> - aux->meter_action = action;
> + /* Records the meter actions. */
> + if (aux->meter_count < EXECUTE_HELPER_MAX_METER) {
> + aux->meter_actions[aux->meter_count++] = action;
> }
> break;
>
> @@ -1197,27 +1200,15 @@ dpif_execute_helper_cb(void *aux_, struct
> dp_packet_batch *packets_,
> uint64_t stub[256 / 8];
> struct pkt_metadata *md = &packet->md;
>
> - if (flow_tnl_dst_is_set(&md->tunnel) || aux->meter_action) {
> + if (flow_tnl_dst_is_set(&md->tunnel) || aux->meter_count) {
> ofpbuf_use_stub(&execute_actions, stub, sizeof stub);
>
> - if (aux->meter_action) {
> - const struct nlattr *a = aux->meter_action;
> -
> - /* XXX: This code collects meter actions since the last
> action
> - * execution via the datapath to be executed right before the
> - * current action that needs to be executed by the datapath.
> - * This is only an approximation, but better than nothing.
> - * Fundamentally, we should have a mechanism by which the
> - * datapath could return the result of the meter action so
> that
> - * we could execute them at the right order. */
> - do {
> + if (aux->meter_count) {
> + size_t i;
> + for (i = 0; i < aux->meter_count; i++) {
> + const struct nlattr *a = aux->meter_actions[i];
> ofpbuf_put(&execute_actions, a, NLA_ALIGN(a->nla_len));
> - /* Find next meter action before 'action', if any. */
> - do {
> - a = nl_attr_next(a);
> - } while (a != action &&
> - nl_attr_type(a) != OVS_ACTION_ATTR_METER);
> - } while (a != action);
> + }
> }
>
> /* The Linux kernel datapath throws away the tunnel information
> @@ -1261,11 +1252,9 @@ dpif_execute_helper_cb(void *aux_, struct
> dp_packet_batch *packets_,
>
> dp_packet_delete(clone);
>
> - if (flow_tnl_dst_is_set(&md->tunnel) || aux->meter_action) {
> + if (flow_tnl_dst_is_set(&md->tunnel) || aux->meter_count) {
> ofpbuf_uninit(&execute_actions);
> -
> - /* Do not re-use the same meters for later output actions. */
> - aux->meter_action = NULL;
> + aux->meter_count = 0;
> }
> break;
> }
> @@ -1303,7 +1292,7 @@ dpif_execute_helper_cb(void *aux_, struct
> dp_packet_batch *packets_,
> static int
> dpif_execute_with_help(struct dpif *dpif, struct dpif_execute *execute)
> {
> - struct dpif_execute_helper_aux aux = {dpif, execute->flow, 0, NULL};
> + struct dpif_execute_helper_aux aux = {dpif, execute->flow, 0, 0, {NULL}};
> struct dp_packet_batch pb;
>
> COVERAGE_INC(dpif_execute_with_help);
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
