[ovs-dev] [PATCH] ofproto-dpif: Fix use-after-free error in handle_miss_upcalls().

Ethan Jackson ethan at nicira.com
Mon Jan 30 21:13:15 UTC 2012


Thanks for tracking this down.  Looks good to me.

Ethan

On Mon, Jan 30, 2012 at 13:10, Ben Pfaff <blp at nicira.com> wrote:
> When handle_flow_miss() saw that subfacet did not have any actions, then
> the associated packet would get freed early, in the loop that constructs
> the set of batched operations.  However, there would still be a "flow_put"
> operation that referenced the key that shares the same memory block as the
> packet.  The memory allocator would overwrite the first few bytes of this
> block, causing bizarre errors in the flow_put.
>
> This commit changes the memory release strategy to be less error-prone, by
> deferring all freeing of packets to the end of the function.  With this
> change, every packet gets freed in the same place, instead of having some
> packets freed in one place and other packets freed in another.
>
> Here is the valgrind report that pinpoints the problem:
>
> Invalid read of size 4
>   at 0x4026838: memcpy (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
>   by 0x80E9B52: dpif_linux_flow_to_ofpbuf (dpif-linux.c:1714)
>   by 0x80E9C77: dpif_linux_operate (dpif-linux.c:883)
>   by 0x80AFB5A: dpif_operate (dpif.c:994)
>   by 0x809A03B: handle_upcalls (ofproto-dpif.c:2758)
>   by 0x809A23A: run_fast (ofproto-dpif.c:757)
>   by 0x808C04E: ofproto_run_fast (ofproto.c:963)
>   by 0x806DFB6: bridge_run_fast (bridge.c:1811)
>   by 0x8074B59: main (ovs-vswitchd.c:98)
>  Address 0x4427948 is 80 bytes inside a block of size 2,048 free'd
>   at 0x402421C: free (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
>   by 0x80CD865: ofpbuf_delete (ofpbuf.c:187)
>   by 0x80CD8AA: ofpbuf_list_delete (ofpbuf.c:531)
>   by 0x8099F06: handle_upcalls (ofproto-dpif.c:2747)
>   by 0x809A23A: run_fast (ofproto-dpif.c:757)
>   by 0x808C04E: ofproto_run_fast (ofproto.c:963)
>   by 0x806DFB6: bridge_run_fast (bridge.c:1811)
>   by 0x8074B59: main (ovs-vswitchd.c:98)
>
> Bug #9346.
> Reported-by: Alan Shieh <ashieh at nicira.com>
> Reported-by: Ethan Jackson <ethan at nicira.com>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
>  ofproto/ofproto-dpif.c |   14 +++++++-------
>  1 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 344f9d4..51d3f3f 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -2579,7 +2579,6 @@ handle_flow_miss(struct ofproto_dpif *ofproto, struct flow_miss *miss,
>             continue;
>         }
>
> -        list_remove(&packet->list_node);
>         if (flow->vlan_tci != subfacet->initial_tci) {
>             /* This packet was received on a VLAN splinter port.  We added
>              * a VLAN to the packet to make the packet resemble the flow,
> @@ -2742,14 +2741,10 @@ handle_miss_upcalls(struct ofproto_dpif *ofproto, struct dpif_upcall *upcalls,
>     /* Process each element in the to-do list, constructing the set of
>      * operations to batch. */
>     n_ops = 0;
> -    HMAP_FOR_EACH_SAFE (miss, next_miss, hmap_node, &todo) {
> +    HMAP_FOR_EACH (miss, hmap_node, &todo) {
>         handle_flow_miss(ofproto, miss, flow_miss_ops, &n_ops);
> -        ofpbuf_list_delete(&miss->packets);
> -        hmap_remove(&todo, &miss->hmap_node);
> -        free(miss);
>     }
>     assert(n_ops <= ARRAY_SIZE(flow_miss_ops));
> -    hmap_destroy(&todo);
>
>     /* Execute batch. */
>     for (i = 0; i < n_ops; i++) {
> @@ -2768,7 +2763,6 @@ handle_miss_upcalls(struct ofproto_dpif *ofproto, struct dpif_upcall *upcalls,
>             if (op->subfacet->actions != execute->actions) {
>                 free((struct nlattr *) execute->actions);
>             }
> -            ofpbuf_delete((struct ofpbuf *) execute->packet);
>             break;
>
>         case DPIF_OP_FLOW_PUT:
> @@ -2778,6 +2772,12 @@ handle_miss_upcalls(struct ofproto_dpif *ofproto, struct dpif_upcall *upcalls,
>             break;
>         }
>     }
> +    HMAP_FOR_EACH_SAFE (miss, next_miss, hmap_node, &todo) {
> +        ofpbuf_list_delete(&miss->packets);
> +        hmap_remove(&todo, &miss->hmap_node);
> +        free(miss);
> +    }
> +    hmap_destroy(&todo);
>  }
>
>  static void
> --
> 1.7.2.5
>



More information about the dev mailing list