diff mbox series

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

Message ID 1737c89ee01142fabdfc62876241e52d@huawei.com
State Superseded
Headers show
Series [ovs-dev] ofproto:fix use-after-free | expand

Commit Message

Hongzhi Guo March 5, 2020, 11:36 a.m. UTC 
ASAN report use-after-free when destroy ofproto_rule, the rule->ofproto has freed in ofproto_destroy.

here is the ASAN report:
================================================================================================

==10399==ERROR: AddressSanitizer: heap-use-after-free on address 0xffff61e1e420 at pc 0xaaaadcc29d1c bp 0xffff6c5fde40 sp 0xffff6c5fde60
READ of size 8 at 0xffff61e1e420 thread T12 (urcu2)
        #0 0xaaaadcc29d1b (/usr/src/debug/openvswitch-2.7.0-1.1.RC5.005.asan.aarch64/ofproto/ofproto.c:2916)
        #1 0xaaaadcf76f5f (/usr/src/debug/openvswitch-2.7.0-1.1.RC5.005.asan.aarch64/lib/ovs-rcu.c:348 (discriminator 3))
        #2 0xaaaadcf770c7 (/usr/src/debug/openvswitch-2.7.0-1.1.RC5.005.asan.aarch64/lib/ovs-rcu.c:363)
        #3 0xaaaadcf7fa9b (/usr/src/debug/openvswitch-2.7.0-1.1.RC5.005.asan.aarch64/lib/ovs-thread.c:708)
        #4 0xffff80fde8bb in start_thread (/lib64/libpthread.so.0+0x78bb)
        #5 0xffff808fa5cb in thread_start (/lib64/libc.so.6+0xd55cb)

0xffff61e1e420 is located 32 bytes inside of 34496-byte region [0xffff61e1e400,0xffff61e26ac0)
freed by thread T12 (urcu2) here:
        #0 0xffff8214fe33 in free (/lib64/libasan.so.4+0xd2e33)
        #1 0xaaaadcc576df (/usr/src/debug/openvswitch-2.7.0-1.1.RC5.005.asan.aarch64/ofproto/ofproto-dpif.c:734)
        #2 0xaaaadcc21acb (/usr/src/debug/openvswitch-2.7.0-1.1.RC5.005.asan.aarch64/ofproto/ofproto.c:1687)
        #3 0xaaaadcf76f5f (/usr/src/debug/openvswitch-2.7.0-1.1.RC5.005.asan.aarch64/lib/ovs-rcu.c:348 (discriminator 3))
        #4 0xaaaadcf770c7 (/usr/src/debug/openvswitch-2.7.0-1.1.RC5.005.asan.aarch64/lib/ovs-rcu.c:363)
        #5 0xaaaadcf7fa9b (/usr/src/debug/openvswitch-2.7.0-1.1.RC5.005.asan.aarch64/lib/ovs-thread.c:708)
        #6 0xffff80fde8bb in start_thread (/lib64/libpthread.so.0+0x78bb)
        #7 0xffff808fa5cb in thread_start (/lib64/libc.so.6+0xd55cb)

previously allocated by thread T0 here:
        #0 0xffff821503c3 in __interceptor_calloc (/lib64/libasan.so.4+0xd33c3)
        #1 0xaaaadd034717 in xcalloc (/usr/src/debug/openvswitch-2.7.0-1.1.RC5.005.asan.aarch64/lib/util.c:99 (discriminator 3))
        #2 0xaaaadd034767 in xzalloc (/usr/src/debug/openvswitch-2.7.0-1.1.RC5.005.asan.aarch64/lib/util.c:110)
        #3 0xaaaadcc576ab (/usr/src/debug/openvswitch-2.7.0-1.1.RC5.005.asan.aarch64/ofproto/ofproto-dpif.c:726)
        #4 0xaaaadcc1be93 in ofproto_create (/usr/src/debug/openvswitch-2.7.0-1.1.RC5.005.asan.aarch64/ofproto/ofproto.c:505)
        #5 0xaaaadcbd793f (/usr/src/debug/openvswitch-2.7.0-1.1.RC5.005.asan.aarch64/vswitchd/bridge.c:1208)
        #6 0xaaaadcbeefb7 in bridge_run (/usr/src/debug/openvswitch-2.7.0-1.1.RC5.005.asan.aarch64/vswitchd/bridge.c:3944 (discriminator 4))
        #7 0xaaaadcbfdb83 in main (/usr/src/debug/openvswitch-2.7.0-1.1.RC5.005.asan.aarch64/vswitchd/ovs-vswitchd.c:240)
        #8 0xffff80845adf in __libc_start_main (/lib64/libc.so.6+0x20adf)
        #9 0xaaaadcbd19b3  (/usr/sbin/ovs-vswitchd-2.7.0-1.1.RC5.005.asan+0x26f9b3)

Should add ref_count for ofproto to avoid use-after-free (rule and group) ?

Appreciate any reply.

Patch as follows:

From c6f3ec805851167f595ab44173a95e2ff43a0499 Mon Sep 17 00:00:00 2001
From: guohongzhi <guohongzhi1@huawei.com>
Date: Thu, 5 Mar 2020 18:40:35 +0800
Subject: [PATCH] add ref_count for ofproto to avoid use-after-free when
destroy ofproto_rule or 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(-)

     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;
}
--
2.21.0.windows.1
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)