diff mbox series

[ovs-dev] ofproto:fix use-after-free

Message ID 20200306130555.19884-1-guohongzhi1@huawei.com
State New
Headers show
Series [ovs-dev] ofproto:fix use-after-free | expand

Commit Message

guohongzhi (A) March 6, 2020, 1:05 p.m. UTC
ASAN report use-after-free when destroy ofproto_rule, the rule->ofproto
has freed in ofproto_destroy.
Add ref_count for ofproto to avoid use-after-free when destroy
ofproto_rule adn group.

Signed-off-by: guohongzhi <guohongzhi1@huawei.com>
---
 ofproto/ofproto-dpif.c     |  1 +
 ofproto/ofproto-provider.h |  3 +++
 ofproto/ofproto.c          | 31 +++++++++++++++++++++++++++++--
 3 files changed, 33 insertions(+), 2 deletions(-)

Comments

Ben Pfaff March 6, 2020, 8:58 p.m. UTC | #1
On Fri, Mar 06, 2020 at 09:05:55PM +0800, guohongzhi wrote:
> ASAN report use-after-free when destroy ofproto_rule, the rule->ofproto
> has freed in ofproto_destroy.
> Add ref_count for ofproto to avoid use-after-free when destroy
> ofproto_rule adn group.
> 
> Signed-off-by: guohongzhi <guohongzhi1@huawei.com>

Why isn't RCU sufficient to avoid use-after-free?
guohongzhi (A) March 9, 2020, 2:35 a.m. UTC | #2
Only RCU may not be sufficient. The deletion of rule and group uses both RCU and reference accounting, but the deletion of ofproto uses only RCU.

The execution process as follows:
ofproto_destroy=>p->ofproto_class->destruct=>ofproto_rule_delete=>ofproto_rule_unref (suppose rule-A’s reference accounting not reach the last, rule-A will not be added to deffered deletion list )=>…=>ofproto_destroy(The ofproto will be added to deferred deletion list directly in the last line of the function)=>soon after,rule-A’s reference accounting reach the last, it will be added to deferred deletion list after oproto. So, ofproto will be released before the rule-A. When the rule_destroy_cb is executed, the internal access of ofproto will cause use-after-free.


-----Original Message-----
From: Ben Pfaff [mailto:blp@ovn.org] 
Sent: 2020年3月7日 4:58
To: guohongzhi (A) <guohongzhi1@huawei.com>
Cc: dev@openvswitch.org; numans@ovn.org; Zhoujingbin (Robin, Russell Lab) <zhoujingbin@huawei.com>; chenchanghu <chenchanghu@huawei.com>; Lilijun (Jerry) <jerry.lilijun@huawei.com>
Subject: Re: [PATCH] [ovs-dev]ofproto:fix use-after-free

On Fri, Mar 06, 2020 at 09:05:55PM +0800, guohongzhi wrote:
> ASAN report use-after-free when destroy ofproto_rule, the 
> rule->ofproto has freed in ofproto_destroy.
> Add ref_count for ofproto to avoid use-after-free when destroy 
> ofproto_rule adn group.
> 
> Signed-off-by: guohongzhi <guohongzhi1@huawei.com>

Why isn't RCU sufficient to avoid use-after-free?
diff mbox series

Patch

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 7515352..258972d 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1494,6 +1494,7 @@  construct(struct ofproto *ofproto_)
     ofproto->ams_seq = seq_create();
     ofproto->ams_seqno = seq_read(ofproto->ams_seq);
 
+    ovs_refcount_init(&ofproto_->ref_count);
 
     SHASH_FOR_EACH_SAFE (node, next, &init_ofp_ports) {
         struct iface_hint *iface_hint = node->data;
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 7907d4b..c3e6527 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -139,6 +139,7 @@  struct ofproto {
     /* Variable length mf_field mapping. Stores all configured variable length
      * meta-flow fields (struct mf_field) in a switch. */
     struct vl_mff_map vl_mff_map;
+    struct ovs_refcount ref_count;
 };
 
 void ofproto_init_tables(struct ofproto *, int n_tables);
@@ -442,6 +443,8 @@  struct rule {
 void ofproto_rule_ref(struct rule *);
 bool ofproto_rule_try_ref(struct rule *);
 void ofproto_rule_unref(struct rule *);
+void ofproto_ref(struct ofproto *ofproto);
+void ofproto_unref(struct ofproto *ofproto);
 
 static inline const struct rule_actions * rule_get_actions(const struct rule *);
 static inline bool rule_is_table_miss(const struct rule *);
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 7b84784..dc9caa0 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -554,6 +554,10 @@  ofproto_create(const char *datapath_name, const char *datapath_type,
         ovs_mutex_unlock(&ofproto_mutex);
         ofproto_destroy__(ofproto);
         return error;
+    } else {
+        /* ofproto construct succeed, ref its self
+         * inorder to unref when call ofproto destroy*/
+        ofproto_ref(ofproto);
     }
 
     /* Check that hidden tables, if any, are at the end. */
@@ -1673,8 +1677,9 @@  ofproto_destroy(struct ofproto *p, bool del)
     p->connmgr = NULL;
     ovs_mutex_unlock(&ofproto_mutex);
 
-    /* Destroying rules is deferred, must have 'ofproto' around for them. */
-    ovsrcu_postpone(ofproto_destroy_defer__, p);
+    /* Inorder to keep the code logical as before, we ref for ofproto when ofproto create
+     * so, we should also unref ofproto here*/
+    ofproto_unref(p);
 }
 
 /* Destroys the datapath with the respective 'name' and 'type'.  With the Linux
@@ -2852,6 +2857,22 @@  update_mtu_ofproto(struct ofproto *p)
     }
 }
 
+void
+ofproto_ref(struct ofproto *ofproto)
+{
+    if (ofproto) {
+        ovs_refcount_ref(&ofproto->ref_count);
+    }
+}
+
+void
+ofproto_unref(struct ofproto *ofproto)
+{
+    if (ofproto && ovs_refcount_unref_relaxed(&ofproto->ref_count) == 1) {
+        ovsrcu_postpone(ofproto_destroy_defer__, ofproto);
+    }
+}
+
 static void
 ofproto_rule_destroy__(struct rule *rule)
     OVS_NO_THREAD_SAFETY_ANALYSIS
@@ -2860,6 +2881,7 @@  ofproto_rule_destroy__(struct rule *rule)
     rule_actions_destroy(rule_get_actions(rule));
     ovs_mutex_destroy(&rule->mutex);
     rule->ofproto->ofproto_class->rule_dealloc(rule);
+    ofproto_unref(rule->ofproto);
 }
 
 static void
@@ -3000,6 +3022,7 @@  group_destroy_cb(struct ofgroup *group)
     ofputil_bucket_list_destroy(CONST_CAST(struct ovs_list *,
                                            &group->buckets));
     group->ofproto->ofproto_class->group_dealloc(group);
+    ofproto_unref(group->ofproto);
 }
 
 void
@@ -5176,6 +5199,7 @@  ofproto_rule_create(struct ofproto *ofproto, struct cls_rule *cr,
 
     /* Initialize base state. */
     *CONST_CAST(struct ofproto **, &rule->ofproto) = ofproto;
+    ovs_refcount_ref(ofproto);
     cls_rule_move(CONST_CAST(struct cls_rule *, &rule->cr), cr);
     ovs_refcount_init(&rule->ref_count);
 
@@ -7271,6 +7295,9 @@  init_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm,
         ofputil_bucket_list_destroy(CONST_CAST(struct ovs_list *,
                                                &(*ofgroup)->buckets));
         ofproto->ofproto_class->group_dealloc(*ofgroup);
+    } else {
+        /* group construct succeed, ref ofproto */
+        ofproto_ref(ofproto);
     }
     return error;
 }