From d3bdf5a715fd7e8684738737ab5602593e1819be Mon Sep 17 00:00:00 2001
From: Ben Pfaff <blp@nicira.com>
Date: Fri, 6 Mar 2009 09:52:08 -0800
Subject: [PATCH] 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.
---
 secchan/ofproto.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/secchan/ofproto.c b/secchan/ofproto.c
index d42a259f..bc632767 100644
--- 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) {
-- 
2.30.2