[ovs-dev] [PATCH] ofproto-dpif-upcall: Fix use of cleared stack memory.

[Ethan Jackson]

Acked-by: Ethan Jackson <ethan at nicira.com>

Thanks for taking care of this.

Ethan

On Fri, Aug 15, 2014 at 1:15 AM, Alex Wang <alexw at nicira.com> wrote:
> Commit cc377352d (ofproto: Reorganize in preparation for direct
> dpdk upcalls.) introduced the bug that uses variable defined on
> the stack inside while loop for reading dpif upcalls and keeps
> reference to attributes of the variable within the same function
> after the stack is cleared.  This bug can cause ovs abort.
>
> This commit fixes the above issue by defining an array of the
> variable on the function stack.
>
> Signed-off-by: Alex Wang <alexw at nicira.com>
> ---
>  ofproto/ofproto-dpif-upcall.c |   27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 180684c..9f68a7d 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -574,55 +574,56 @@ recv_upcalls(struct handler *handler)
>      struct udpif *udpif = handler->udpif;
>      uint64_t recv_stubs[UPCALL_MAX_BATCH][512 / 8];
>      struct ofpbuf recv_bufs[UPCALL_MAX_BATCH];
> +    struct dpif_upcall dupcalls[UPCALL_MAX_BATCH];
>      struct upcall upcalls[UPCALL_MAX_BATCH];
>      size_t n_upcalls, i;
>
>      n_upcalls = 0;
>      while (n_upcalls < UPCALL_MAX_BATCH) {
>          struct ofpbuf *recv_buf = &recv_bufs[n_upcalls];
> +        struct dpif_upcall *dupcall = &dupcalls[n_upcalls];
>          struct upcall *upcall = &upcalls[n_upcalls];
> -        struct dpif_upcall dupcall;
>          struct pkt_metadata md;
>          struct flow flow;
>          int error;
>
>          ofpbuf_use_stub(recv_buf, recv_stubs[n_upcalls],
>                          sizeof recv_stubs[n_upcalls]);
> -        if (dpif_recv(udpif->dpif, handler->handler_id, &dupcall, recv_buf)) {
> +        if (dpif_recv(udpif->dpif, handler->handler_id, dupcall, recv_buf)) {
>              ofpbuf_uninit(recv_buf);
>              break;
>          }
>
> -        if (odp_flow_key_to_flow(dupcall.key, dupcall.key_len, &flow)
> +        if (odp_flow_key_to_flow(dupcall->key, dupcall->key_len, &flow)
>              == ODP_FIT_ERROR) {
>              goto free_dupcall;
>          }
>
> -        error = upcall_receive(upcall, udpif->backer, &dupcall.packet,
> -                               dupcall.type, dupcall.userdata, &flow);
> +        error = upcall_receive(upcall, udpif->backer, &dupcall->packet,
> +                               dupcall->type, dupcall->userdata, &flow);
>          if (error) {
>              if (error == ENODEV) {
>                  /* Received packet on datapath port for which we couldn't
>                   * associate an ofproto.  This can happen if a port is removed
>                   * while traffic is being received.  Print a rate-limited
>                   * message in case it happens frequently. */
> -                dpif_flow_put(udpif->dpif, DPIF_FP_CREATE, dupcall.key,
> -                              dupcall.key_len, NULL, 0, NULL, 0, NULL);
> +                dpif_flow_put(udpif->dpif, DPIF_FP_CREATE, dupcall->key,
> +                              dupcall->key_len, NULL, 0, NULL, 0, NULL);
>                  VLOG_INFO_RL(&rl, "received packet on unassociated datapath "
>                               "port %"PRIu32, flow.in_port.odp_port);
>              }
>              goto free_dupcall;
>          }
>
> -        upcall->key = dupcall.key;
> -        upcall->key_len = dupcall.key_len;
> +        upcall->key = dupcall->key;
> +        upcall->key_len = dupcall->key_len;
>
> -        if (vsp_adjust_flow(upcall->ofproto, &flow, &dupcall.packet)) {
> +        if (vsp_adjust_flow(upcall->ofproto, &flow, &dupcall->packet)) {
>              upcall->vsp_adjusted = true;
>          }
>
>          md = pkt_metadata_from_flow(&flow);
> -        flow_extract(&dupcall.packet, &md, &flow);
> +        flow_extract(&dupcall->packet, &md, &flow);
>
>          error = process_upcall(udpif, upcall, NULL);
>          if (error) {
> @@ -635,14 +636,14 @@ recv_upcalls(struct handler *handler)
>  cleanup:
>          upcall_uninit(upcall);
>  free_dupcall:
> -        ofpbuf_uninit(&dupcall.packet);
> +        ofpbuf_uninit(&dupcall->packet);
>          ofpbuf_uninit(recv_buf);
>      }
>
>      if (n_upcalls) {
>          handle_upcalls(handler->udpif, upcalls, n_upcalls);
>          for (i = 0; i < n_upcalls; i++) {
> -            ofpbuf_uninit(CONST_CAST(struct ofpbuf *, upcalls[i].packet));
> +            ofpbuf_uninit(&dupcalls[i].packet);
>              ofpbuf_uninit(&recv_bufs[i]);
>              upcall_uninit(&upcalls[i]);
>          }
> --
> 1.7.9.5
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev