secchan: Fix use-after-free by allocating rule actions as separate blocks.

The ofproto code tried to cleverly save memory and time by allocating
rule actions as a flexible array member at the end of "struct rule".  When
the actions changed, this required a realloc() call.  Unfortunately, there
are sometimes pointers to rules (e.g. the "super" pointer from subrules)
that were not getting adjusted to point to the new location.

It's better to just allocate actions separately, so fix it by doing that.

diff --git a/secchan/ofproto.c b/secchan/ofproto.c
index d42a259fe5c494408a7b1ca4fb5b6d5b677ddce5..bc632767dd4c61264a0a1bac96d0903245f31d7f 100644 (file)
--- a/secchan/ofproto.c
+++ b/secchan/ofproto.c
@@ -115,11 +115,10 @@ struct rule {
 
     /* A subrule has no actions (it uses the super-rule's actions). */
     int n_actions;
-    union ofp_action actions[];
+    union ofp_action *actions;
 };
 
 static void rule_destroy(struct rule *);
-static inline size_t rule_size(int n_actions);
 static struct rule *rule_from_cls_rule(const struct cls_rule *);
 static void rule_make_actions(struct ofproto *,
                               const struct rule *, struct odp_actions *);
@@ -762,7 +761,7 @@ ofproto_setup_exact_flow(struct ofproto *p, const flow_t *flow,
     struct odp_actions odp_actions;
     struct odp_flow odp_flow;
 
-    rule = xmalloc(rule_size(n_actions));
+    rule = xmalloc(sizeof *rule);
     cls_rule_from_flow(&rule->cr, flow, 0, UINT16_MAX);
     rule->idle_timeout = 5;     /* XXX */
     rule->hard_timeout = 0;     /* XXX */
@@ -774,7 +773,7 @@ ofproto_setup_exact_flow(struct ofproto *p, const flow_t *flow,
     rule->super = NULL;         /* XXX */
     list_init(&rule->list);
     rule->n_actions = n_actions;
-    memcpy(rule->actions, actions, n_actions * sizeof *rule->actions);
+    rule->actions = xmemdup(actions, n_actions * sizeof *rule->actions);
 
     displaced_rule = rule_from_cls_rule(classifier_insert(&p->cls, &rule->cr));
     if (displaced_rule) {
@@ -1122,6 +1121,7 @@ rule_destroy(struct rule *rule)
     } else if (rule->super != UNKNOWN_SUPER) {
         list_remove(&rule->list);
     }
+    free(rule->actions);
     free(rule);
 }
 
@@ -2099,7 +2099,7 @@ add_flow(struct ofproto *p, struct ofconn *ofconn,
     struct rule *rule, *displaced_rule;
     int buffer_error = 0;
 
-    rule = xmalloc(rule_size(n_actions));
+    rule = xmalloc(sizeof *rule);
     cls_rule_from_match(&rule->cr, &ofm->match, ntohs(ofm->priority));
     rule->idle_timeout = ntohs(ofm->idle_timeout);
     rule->hard_timeout = ntohs(ofm->hard_timeout);
@@ -2111,7 +2111,7 @@ add_flow(struct ofproto *p, struct ofconn *ofconn,
     rule->super = NULL;
     list_init(&rule->list);
     rule->n_actions = n_actions;
-    memcpy(rule->actions, ofm->actions, n_actions * sizeof *rule->actions);
+    rule->actions = xmemdup(ofm->actions, n_actions * sizeof *rule->actions);
 
     if (ofm->buffer_id != htonl(UINT32_MAX)) {
         int byte_count = 0;
@@ -2199,7 +2199,6 @@ modify_flow(struct ofproto *p, const struct ofp_flow_mod *ofm,
         classifier_remove(&p->cls, &rule->cr);
         rule_destroy(rule);
     } else {
-        struct rule *old_rule = rule;
         if (!rule->cr.wc.wildcards) {
             struct odp_flow odp_flow;
             struct odp_actions actions;
@@ -2214,10 +2213,10 @@ modify_flow(struct ofproto *p, const struct ofp_flow_mod *ofm,
 
             update_stats(rule, &odp_flow.stats);
         }
-        rule = xrealloc(rule, rule_size(n_actions));
+        free(rule->actions);
+        rule->actions = xmemdup(ofm->actions,
+                                n_actions * sizeof *rule->actions);
         rule->n_actions = n_actions;
-        memcpy(rule->actions, ofm->actions, n_actions * sizeof *rule->actions);
-        cls_rule_moved(&p->cls, &old_rule->cr, &rule->cr);
     }
 
     return 0;
@@ -2495,6 +2494,7 @@ handle_odp_msg(struct ofproto *p, struct ofpbuf *packet)
         subrule->ip_tos = 0;
         subrule->super = rule;
         subrule->n_actions = 0;
+        subrule->actions = NULL;
 
         old_sr = rule_from_cls_rule(classifier_insert(&p->cls, &subrule->cr));
         if (old_sr) {