secchan: Fix segfault due to access-after-free in expiration.

classifier_for_each() keeps a pointer to the *next* rule to be visited, so
that the rule currently be visited can be deleted.  That means that if
the callback frees the next rule to be visited, then we get an
access-after-free error.

In particular, this was occurring when expire_rule() expired a superflow
whose

diff --git a/lib/classifier.c b/lib/classifier.c
index ddc5bf0eed8ce32dfca9d8a545c145785eef21a7..7f10aa24b1042091335b75df259d191c6ab521cc 100644 (file)
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -405,33 +405,43 @@ classifier_for_each_match(const struct classifier *cls,
     }
 }
 
+/* 'callback' is allowed to delete the rule that is passed as its argument, but
+ * it must not delete (or move) any other rules in 'cls' that will be visited
+ * by this call to classifier_for_each().  (It must not even delete rules that
+ * would fall into other tables, e.g. exact-match rules if the argument rule
+ * has wildcards.) */
 void
-classifier_for_each(const struct classifier *cls,
+classifier_for_each(const struct classifier *cls, int include,
                     void (*callback)(struct cls_rule *, void *aux),
                     void *aux)
 {
-    struct cls_bucket *bucket, *next_bucket;
     struct cls_rule *prev_rule, *rule, *next_rule;
     const struct hmap *tbl;
 
     prev_rule = NULL;
-    for (tbl = &cls->tables[0]; tbl < &cls->tables[CLS_N_FIELDS]; tbl++) {
-        HMAP_FOR_EACH_SAFE (bucket, next_bucket,
-                            struct cls_bucket, hmap_node, tbl) {
-            LIST_FOR_EACH (rule, struct cls_rule, node.list, &bucket->rules) {
-                if (prev_rule) {
-                    callback(prev_rule, aux);
+    if (include & CLS_INC_WILD) {
+        struct cls_bucket *bucket, *next_bucket;
+        for (tbl = &cls->tables[0]; tbl < &cls->tables[CLS_N_FIELDS]; tbl++) {
+            HMAP_FOR_EACH_SAFE (bucket, next_bucket,
+                                struct cls_bucket, hmap_node, tbl) {
+                LIST_FOR_EACH (rule, struct cls_rule, node.list,
+                               &bucket->rules) {
+                    if (prev_rule) {
+                        callback(prev_rule, aux);
+                    }
+                    prev_rule = rule;
                 }
-                prev_rule = rule;
             }
         }
     }
-    HMAP_FOR_EACH_SAFE (rule, next_rule,
-                        struct cls_rule, node.hmap, &cls->exact_table) {
-        if (prev_rule) {
-            callback(prev_rule, aux);
+    if (include & CLS_INC_EXACT) {
+        HMAP_FOR_EACH_SAFE (rule, next_rule,
+                            struct cls_rule, node.hmap, &cls->exact_table) {
+            if (prev_rule) {
+                callback(prev_rule, aux);
+            }
+            prev_rule = rule;
         }
-        prev_rule = rule;
     }
     if (prev_rule) {
         callback(prev_rule, aux);

diff --git a/lib/classifier.h b/lib/classifier.h
index fdbb7bb83cba8ef6b488ec4e5032e4ca94155a31..981d03ab28af61508224be22326e8b708d28510a 100644 (file)
--- a/lib/classifier.h
+++ b/lib/classifier.h
@@ -146,15 +146,17 @@ struct cls_rule *classifier_lookup_exact(const struct classifier *,
                                          const flow_t *);
 
 typedef void cls_cb_func(struct cls_rule *, void *aux);
-void classifier_for_each(const struct classifier *, cls_cb_func *, void *aux);
-void classifier_for_each_with_wildcards(const struct classifier *,
-                                        uint32_t wildcards,
-                                        cls_cb_func *, void *aux);
 
 enum {
     CLS_INC_EXACT = 1 << 0,     /* Include exact-match flows? */
     CLS_INC_WILD = 1 << 1       /* Include flows with wildcards? */
 };
+void classifier_for_each(const struct classifier *, int include,
+                         cls_cb_func *, void *aux);
+void classifier_for_each_with_wildcards(const struct classifier *,
+                                        uint32_t wildcards,
+                                        cls_cb_func *, void *aux);
+
 void classifier_for_each_match(const struct classifier *,
                                const struct cls_rule *,
                                int include, cls_cb_func *, void *aux);

diff --git a/secchan/ofproto.c b/secchan/ofproto.c
index 1a3b4e8c952c1bc2cc6da372faa1ac390bf10feb..7b2b468e69ae9f202380b157a1fc2d4a14471325 100644 (file)
--- a/secchan/ofproto.c
+++ b/secchan/ofproto.c
@@ -429,12 +429,18 @@ ofproto_run(struct ofproto *p)
     if (time_msec() >= p->next_expiration) {
         p->next_expiration = time_msec() + 1000;
         update_used(p);
-        classifier_for_each(&p->cls, expire_rule, p);
+
+        /* This is broken into two calls to classifier_for_each() due to that
+         * function's requirement that the callback not delete any rules that
+         * will be visited, other than the callback's argument.  In particular,
+         * calling expire_rule() on a rule with wildcards can delete an
+         * arbitrary number of exact-match rules. */
+        classifier_for_each(&p->cls, CLS_INC_WILD, expire_rule, p);
+        classifier_for_each(&p->cls, CLS_INC_EXACT, expire_rule, p);
     }
 
     if (p->need_revalidate) {
-        classifier_for_each_with_wildcards(&p->cls, 0, revalidate_subrule_cb,
-                                           p);
+        classifier_for_each(&p->cls, CLS_INC_EXACT, revalidate_subrule_cb, p);
         p->need_revalidate = false;
     }
 }

diff --git a/tests/test-classifier.c b/tests/test-classifier.c
index c4d7f57c06d13ee3b7b3f501d578d91462c783d8..03936656f6e47b75328a0ce94e20e33ca806c299 100644 (file)
--- a/tests/test-classifier.c
+++ b/tests/test-classifier.c
@@ -403,7 +403,7 @@ free_rule(struct cls_rule *cls_rule, void *cls)
 static void
 destroy_classifier(struct classifier *cls)
 {
-    classifier_for_each(cls, free_rule, cls);
+    classifier_for_each(cls, CLS_INC_WILD | CLS_INC_EXACT, free_rule, cls);
     classifier_destroy(cls);
 }