From patchwork Thu Mar 5 11:36:39 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hongzhi Guo X-Patchwork-Id: 1249516 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=140.211.166.138; helo=whitealder.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=huawei.com Received: from whitealder.osuosl.org (smtp1.osuosl.org [140.211.166.138]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 48Y7xx0C9zz9sPK for ; Thu, 5 Mar 2020 22:36:52 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id 0A82586AF9; Thu, 5 Mar 2020 11:36:50 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from whitealder.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Lz2pu99Uc8GI; Thu, 5 Mar 2020 11:36:47 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by whitealder.osuosl.org (Postfix) with ESMTP id 04C258696A; Thu, 5 Mar 2020 11:36:47 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id DD449C18DA; Thu, 5 Mar 2020 11:36:46 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137]) by lists.linuxfoundation.org (Postfix) with ESMTP id 8B88EC013E for ; Thu, 5 Mar 2020 11:36:45 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id 875BF84C33 for ; Thu, 5 Mar 2020 11:36:45 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from fraxinus.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 7577ExsC5px2 for ; Thu, 5 Mar 2020 11:36:43 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from huawei.com (szxga08-in.huawei.com [45.249.212.255]) by fraxinus.osuosl.org (Postfix) with ESMTPS id 364AD8459B for ; Thu, 5 Mar 2020 11:36:43 +0000 (UTC) Received: from DGGEMM403-HUB.china.huawei.com (unknown [172.30.72.57]) by Forcepoint Email with ESMTP id CBEBCCF274C3F0A99C6E; Thu, 5 Mar 2020 19:36:39 +0800 (CST) Received: from dggeme751-chm.china.huawei.com (10.3.19.97) by DGGEMM403-HUB.china.huawei.com (10.3.20.211) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 5 Mar 2020 19:36:39 +0800 Received: from dggeme752-chm.china.huawei.com (10.3.19.98) by dggeme751-chm.china.huawei.com (10.3.19.97) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.1713.5; Thu, 5 Mar 2020 19:36:39 +0800 Received: from dggeme752-chm.china.huawei.com ([10.6.80.76]) by dggeme752-chm.china.huawei.com ([10.6.80.76]) with mapi id 15.01.1713.004; Thu, 5 Mar 2020 19:36:39 +0800 From: "guohongzhi (A)" To: Numan Siddique , Ben Pfaff Thread-Topic: [ovs-dev][patch]ofproto:fix use-after-free Thread-Index: AdXy4Pr/4hOf0EAyT+SkWuWc9FfHLw== Date: Thu, 5 Mar 2020 11:36:39 +0000 Message-ID: <1737c89ee01142fabdfc62876241e52d@huawei.com> Accept-Language: en-US Content-Language: zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.173.251.143] MIME-Version: 1.0 X-CFilter-Loop: Reflected X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Cc: "dev@openvswitch.org" , "Zhoujingbin \(Robin, Russell Lab\)" , chenchanghu , Guohongzhi Subject: [ovs-dev] [patch]ofproto:fix use-after-free X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" 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 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 --- 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 --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)