From: Ben Pfaff <blp@nicira.com>
Date: Wed, 4 Mar 2009 23:08:57 +0000 (-0800)
Subject: secchan: Fix segfault due to access-after-free in expiration.
X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=bc2658d4c9fbd7dd1bf4903ef7c5153c867c83b8;p=openvswitch

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 ddc5bf0e..7f10aa24 100644
--- 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 fdbb7bb8..981d03ab 100644
--- 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 1a3b4e8c..7b2b468e 100644
--- 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 c4d7f57c..03936656 100644
--- 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);
 }