[ovs-dev] [PATCH] ofp-actions: Fix use-after-free with ofpact_finish().

[Joe Stringer]

ofpact_finish() may now reallocate the buffer it is passed, but not all
callers updated their local pointers to the current action in the
buffer. This could potentially lead to several use-after-free bugs.

Update ofpact_finish() to return the new pointer to the ofpact which is
provided, and update the calling points to ensure that their local
pointers are pointing into the correct (potentially reallocated) buffer.

Fixes: 2bd318dec242 ("ofp-actions: Make composing actions harder to screw up.")
Reported-by: William Tu <u9012063 at gmail.com>
Signed-off-by: Joe Stringer <joe at ovn.org>
---
 lib/bundle.c           |  4 +---
 lib/ofp-actions.c      | 14 +++++++++-----
 lib/ofp-actions.h      |  2 +-
 ofproto/ofproto-dpif.c |  2 +-
 4 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/lib/bundle.c b/lib/bundle.c
index 1451e928fec6..07dc9f4919b2 100644
--- a/lib/bundle.c
+++ b/lib/bundle.c
@@ -178,9 +178,7 @@ bundle_parse__(const char *s, char **save_ptr,
         bundle = ofpacts->header;
         bundle->n_slaves++;
     }
-    ofpact_finish(ofpacts, &bundle->ofpact);
-
-    bundle = ofpacts->header;
+    bundle = ofpact_finish(ofpacts, &bundle->ofpact);
     bundle->basis = atoi(basis);
 
     if (!strcasecmp(fields, "eth_src")) {
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 905469b6bbf9..a7c0388adeaa 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -1256,8 +1256,7 @@ decode_bundle(bool load, const struct nx_action_bundle *nab,
         bundle = ofpacts->header;
     }
 
-    ofpact_finish(ofpacts, &bundle->ofpact);
-
+    bundle = ofpact_finish(ofpacts, &bundle->ofpact);
     if (!error) {
         error = bundle_check(bundle, OFPP_MAX, NULL);
     }
@@ -4956,7 +4955,7 @@ decode_NXAST_RAW_CT(const struct nx_action_conntrack *nac,
 
     conntrack = ofpbuf_push_uninit(out, sizeof(*conntrack));
     out->header = &conntrack->ofpact;
-    ofpact_finish(out, &conntrack->ofpact);
+    conntrack = ofpact_finish(out, &conntrack->ofpact);
 
     if (conntrack->ofpact.len > sizeof(*conntrack)
         && !(conntrack->flags & NX_CT_F_COMMIT)) {
@@ -7397,8 +7396,11 @@ ofpact_init(struct ofpact *ofpact, enum ofpact_type type, size_t len)
 /* Finishes composing a variable-length action (begun using
  * ofpact_put_<NAME>()), by padding the action to a multiple of OFPACT_ALIGNTO
  * bytes and updating its embedded length field.  See the large comment near
- * the end of ofp-actions.h for more information. */
-void
+ * the end of ofp-actions.h for more information.
+ *
+ * May reallocate 'ofpacts'. Callers should consider updating their 'ofpact'
+ * pointer to the return value of this function. */
+void *
 ofpact_finish(struct ofpbuf *ofpacts, struct ofpact *ofpact)
 {
     ptrdiff_t len;
@@ -7408,6 +7410,8 @@ ofpact_finish(struct ofpbuf *ofpacts, struct ofpact *ofpact)
     ovs_assert(len <= UINT16_MAX);
     ofpact->len = len;
     ofpbuf_padto(ofpacts, OFPACT_ALIGN(ofpacts->size));
+
+    return ofpacts->header;
 }
 
 static char * OVS_WARN_UNUSED_RESULT
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index 24143d324323..ec8b36981a56 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -968,7 +968,7 @@ OFPACTS
 #undef OFPACT
 
 /* Call after adding the variable-length part to a variable-length action. */
-void ofpact_finish(struct ofpbuf *, struct ofpact *);
+void *ofpact_finish(struct ofpbuf *, struct ofpact *);
 
 /* Additional functions for composing ofpacts. */
 struct ofpact_set_field *ofpact_put_reg_load(struct ofpbuf *ofpacts);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index b963ff201782..69521a63a448 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1411,7 +1411,7 @@ add_internal_flows(struct ofproto_dpif *ofproto)
     controller->max_len = UINT16_MAX;
     controller->controller_id = 0;
     controller->reason = OFPR_IMPLICIT_MISS;
-    ofpact_finish(&ofpacts, &controller->ofpact);
+    controller = ofpact_finish(&ofpacts, &controller->ofpact);
 
     error = add_internal_miss_flow(ofproto, id++, &ofpacts,
                                    &ofproto->miss_rule);
-- 
2.1.4