From: Ben Pfaff <blp@nicira.com>
Date: Mon, 30 Jan 2012 21:09:04 +0000 (-0800)
Subject: ofproto-dpif: Fix use-after-free error in handle_miss_upcalls().
X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=33bb0caa64ccd8c7424eb0d4f6f4f068a17e99ff;p=openvswitch

ofproto-dpif: Fix use-after-free error in handle_miss_upcalls().

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@nicira.com>
Reported-by: Ethan Jackson <ethan@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
---

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 344f9d4d..51d3f3f5 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