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

[Ben Pfaff]

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