[ovs-dev] [branch-2.3 v2] ofproto-dpif-upcall: Fix additional use-after-free in revalidate().

Ben Pfaff blp at nicira.com
Thu May 22 04:15:07 UTC 2014


Thanks for the review.  Ordinarily I would spend some time refining the
commit message to better explain, because I like commit messages to be
clear to everyone, but I am feeling a lot of pressure to fix this bug,
so I pushed it as is.

On Thu, May 22, 2014 at 02:24:37PM +1200, Joe Stringer wrote:
> The commit message is a bit confusing to me, I don't see the relationship
> to commit 1340ce0c175. I see it more as a discrepancy between the behaviour
> of dpif_linux_flow_dump_next_may_destroy_keys() and
> dpif_linux_flow_dump_next(). However I agree with the rest of the commit
> message.
> 
> Tested briefly, didn't check it fixes the original bug.
> 
> Acked-by: Joe Stringer <joestringer at nicira.com>
> 
> 
> On 22 May 2014 11:26, Ben Pfaff <blp at nicira.com> wrote:
> 
> > Commit 1340ce0c175 (ofproto-dpif-upcall: Avoid use-after-free in
> > revalidate() corner cases.) fixed one use-after-free error in revalidate(),
> > but missed one more subtle case, in which dpif_linux_flow_dump_next()
> > attempts to retrieve actions for a flow that didn't have them in the main
> > dump result.  If retrieving those actions fails,
> > dpif_linux_flow_dump_next() goes on to the next flow, and as part of that
> > overwrites the old dumped flows in its buffer.  This is a problem because
> > dpif_linux_flow_dump_next_may_destroy_keys() would have indicated that
> > the old dumped flows would not have been destroyed, which means that the
> > data the caller relied on has gone away.  In the worst case, this causes
> > a segfault and core dump.
> >
> > The best way to fix this problem is the refactoring that has already
> > happened on master in commit ac64794acb85 (dpif: Refactor flow dumping
> > interface to make better sense for batching.)  That is a fairly large
> > change, and not yet well-tested, so it doesn't yet seem suitable for a
> > stable branch.  For now, this commit refines the conditions that
> > dpif_linux_flow_dump_next_may_destroy_keys() uses, so that if the next
> > flow does not include actions it indicates that keys may be destroyed.
> >
> > Thanks to Joe Stringer for suggesting this particular solution.
> >
> > Bug #1249988.
> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> > ---
> >  lib/dpif-linux.c     |   12 ++++++++++--
> >  lib/netlink-socket.c |   13 +++++++++++++
> >  lib/netlink-socket.h |    1 +
> >  3 files changed, 24 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
> > index abb4b51..4af0607 100644
> > --- a/lib/dpif-linux.c
> > +++ b/lib/dpif-linux.c
> > @@ -1256,8 +1256,16 @@ static bool
> >  dpif_linux_flow_dump_next_may_destroy_keys(void *state_)
> >  {
> >      struct dpif_linux_flow_state *state = state_;
> > -
> > -    return ofpbuf_size(&state->buffer) ? false : true;
> > +    struct dpif_linux_flow flow;
> > +    struct ofpbuf nlmsg;
> > +
> > +    /* Check whether there's a flow remaining in the buffer that includes
> > +     * actions.  (If it does not include actions, then we could end up
> > +     * destroying keys previously returned trying to retrieve its actions
> > +     * fails.) */
> > +    return (!nl_dump_peek(&nlmsg, &state->buffer)
> > +            || dpif_linux_flow_from_ofpbuf(&flow, &nlmsg)
> > +            || !flow.actions);
> >  }
> >
> >  static int
> > diff --git a/lib/netlink-socket.c b/lib/netlink-socket.c
> > index e4cc4ad..ef476f2 100644
> > --- a/lib/netlink-socket.c
> > +++ b/lib/netlink-socket.c
> > @@ -797,6 +797,19 @@ exit:
> >      return !error;
> >  }
> >
> > +/* Attempts to look ahead in 'buffer' to obtain the next reply that will
> > be
> > + * returned by nl_dump_next().  Returns true if successful, in which case
> > + * 'reply' will be initialize to the message that will be obtained by the
> > next
> > + * call to nl_dump_next(), or false on failure.  Failure doesn't
> > necessarily
> > + * mean that the nl_dump_next() will fail, only that it needs to obtain a
> > new
> > + * block of dump results from the kernel. */
> > +bool
> > +nl_dump_peek(struct ofpbuf *reply, struct ofpbuf *buffer)
> > +{
> > +    struct ofpbuf tmp = *buffer;
> > +    return nl_msg_next(&tmp, reply);
> > +}
> > +
> >  /* Completes Netlink dump operation 'dump', which must have been
> > initialized
> >   * with nl_dump_start().  Returns 0 if the dump operation was error-free,
> >   * otherwise a positive errno value describing the problem. */
> > diff --git a/lib/netlink-socket.h b/lib/netlink-socket.h
> > index dd32409..1f06b97 100644
> > --- a/lib/netlink-socket.h
> > +++ b/lib/netlink-socket.h
> > @@ -119,6 +119,7 @@ struct nl_dump {
> >  void nl_dump_start(struct nl_dump *, int protocol,
> >                     const struct ofpbuf *request);
> >  bool nl_dump_next(struct nl_dump *, struct ofpbuf *reply, struct ofpbuf
> > *buf);
> > +bool nl_dump_peek(struct ofpbuf *reply, struct ofpbuf *buf);
> >  int nl_dump_done(struct nl_dump *);
> >
> >  /* Miscellaneous */
> > --
> > 1.7.10.4
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
> >



More information about the dev mailing list