ofp-actions: Fix use-after-free with ofpact_finish().

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@gmail.com>
Signed-off-by: Joe Stringer <joe@ovn.org>
Acked-by: Ben Pfaff <blp@ovn.org>
Acked-by: Ryan Moats <rmoats@us.ibm.com>

3 files changed, 11 insertions, 9 deletions

diff --git a/lib/bundle.c b/lib/bundle.c
index 69249e6c6..09a8eb9a6 100644
--- a/lib/bundle.c
+++ b/lib/bundle.c
@@ -179,9 +179,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 0438c6202..98227f720 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -1262,8 +1262,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);
     }
@@ -4995,7 +4994,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)) {
@@ -7443,8 +7442,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;
@@ -7454,6 +7456,8 @@ ofpact_finish(struct ofpbuf *ofpacts, struct ofpact *ofpact)
     ovs_assert(len > 0 && 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 46818e33b..4bd8854f7 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -970,7 +970,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);