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 <u9012...@gmail.com>
Signed-off-by: Joe Stringer <j...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
