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

Ben Pfaff blp at nicira.com
Mon Jan 30 21:20:05 UTC 2012


Thank you.  I pushed the fix to master and branch-1.5.

On Mon, Jan 30, 2012 at 01:13:15PM -0800, Ethan Jackson wrote:
> 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