Skip to content

Commit

Permalink
ofproto:fix use-after-free of ofproto
Browse files Browse the repository at this point in the history 
ASAN report use-after-free of ofproto when destroy ofproto_rule.
The rule uses both RCU and refcount, while the ofproto uses only RCU,
and the rule retains the pointer of the proto.
More importantly, ofproto cannot guarantee a longer grace period than the rule.
So when the rule is deleted, it is possible that ofproto has been released,
resulting in use-after-free of ofproto.
This patch add ref_count for ofproto to avoid use-after-free.

===================
ASAN report as following:
==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.12.asan.aarch64/ofproto/ofproto.c:2916)
    openvswitch#1 0xaaaadcf76f5f (/usr/src/debug/openvswitch-2.12.asan.aarch64/lib/ovs-rcu.c:348 (discriminator 3))
    openvswitch#2 0xaaaadcf770c7 (/usr/src/debug/openvswitch-2.12.asan.aarch64/lib/ovs-rcu.c:363)
    openvswitch#3 0xaaaadcf7fa9b (/usr/src/debug/openvswitch-2.12.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)
    openvswitch#1 0xaaaadcc576df (/usr/src/debug/openvswitch-2.12.asan.aarch64/ofproto/ofproto-dpif.c:734)
    openvswitch#2 0xaaaadcc21acb (/usr/src/debug/openvswitch-2.12.asan.aarch64/ofproto/ofproto.c:1687)
    openvswitch#3 0xaaaadcf76f5f (/usr/src/debug/openvswitch-2.12.asan.aarch64/lib/ovs-rcu.c:348 (discriminator 3))
    #4 0xaaaadcf770c7 (/usr/src/debug/openvswitch-2.12.asan.aarch64/lib/ovs-rcu.c:363)
    #5 0xaaaadcf7fa9b (/usr/src/debug/openvswitch-2.12.asan.aarch64/lib/ovs-thread.c:708)
    openvswitch#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)
    openvswitch#1 0xaaaadd034717 in xcalloc (/usr/src/debug/openvswitch-2.12.asan.aarch64/lib/util.c:99 (discriminator 3))
    openvswitch#2 0xaaaadd034767 in xzalloc (/usr/src/debug/openvswitch-2.12.asan.aarch64/lib/util.c:110)
    openvswitch#3 0xaaaadcc576ab (/usr/src/debug/openvswitch-2.12.asan.aarch64/ofproto/ofproto-dpif.c:726)
    #4 0xaaaadcc1be93 in ofproto_create (/usr/src/debug/openvswitch-2.12.asan.aarch64/ofproto/ofproto.c:505)
    #5 0xaaaadcbd793f (/usr/src/debug/openvswitch-2.12.asan.aarch64/vswitchd/bridge.c:1208)
    openvswitch#6 0xaaaadcbeefb7 in bridge_run (/usr/src/debug/openvswitch-2.12.asan.aarch64/vswitchd/bridge.c:3944 (discriminator 4))
    #7 0xaaaadcbfdb83 in main (/usr/src/debug/openvswitch-2.12.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.12.asan+0x26f9b3)
    SUMMARY: AddressSanitizer: heap-use-after-free (/usr/src/debug/openvswitch-2.12.asan.aarch64/ofproto/ofproto.c:2916)

===================
TEST:
1.ASAN test for three days

CC: Jarno Rajahalme <jrajahalme@nicira.com>
Fixes: 39c9459 ("Use classifier versioning.")

Signed-off-by: Hongzhi Guo <guohongzhi1@huawei.com>
Signed-off-by: hepeng <xnhp0320@gmail.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
  • Loading branch information
@ovsrobot
Hongzhi Guo authored and ovsrobot committed Mar 8, 2021
1 parent cdaa7e0 commit 8478676
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 12 deletions.
1 change: 1 addition & 0 deletions ofproto/ofproto-dpif-xlate-cache.c
View file
Expand Up @@ -209,6 +209,7 @@ xlate_cache_clear_entry(struct xc_entry *entry)
{
switch (entry->type) {
case XC_TABLE:
ofproto_unref(&(entry->table.ofproto->up));
break;
case XC_RULE:
ofproto_rule_unref(&entry->rule->up);
Expand Down
20 changes: 12 additions & 8 deletions ofproto/ofproto-dpif.c
View file
Expand Up @@ -4416,10 +4416,12 @@ rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto,
if (xcache) {
struct xc_entry *entry;

entry = xlate_cache_add_entry(xcache, XC_TABLE);
entry->table.ofproto = ofproto;
entry->table.id = *table_id;
entry->table.match = true;
if (ofproto_try_ref(&ofproto->up)) {
entry = xlate_cache_add_entry(xcache, XC_TABLE);
entry->table.ofproto = ofproto;
entry->table.id = *table_id;
entry->table.match = true;
}
}
return rule;
}
Expand Down Expand Up @@ -4452,10 +4454,12 @@ rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto,
if (xcache) {
struct xc_entry *entry;

entry = xlate_cache_add_entry(xcache, XC_TABLE);
entry->table.ofproto = ofproto;
entry->table.id = next_id;
entry->table.match = (rule != NULL);
if (ofproto_try_ref(&ofproto->up)) {
entry = xlate_cache_add_entry(xcache, XC_TABLE);
entry->table.ofproto = ofproto;
entry->table.id = next_id;
entry->table.match = (rule != NULL);
}
}
if (rule) {
goto out; /* Match. */
Expand Down
5 changes: 5 additions & 0 deletions ofproto/ofproto-provider.h
View file
Expand Up @@ -139,6 +139,8 @@ 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);
Expand Down Expand Up @@ -442,6 +444,9 @@ 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 *);
bool ofproto_try_ref(struct ofproto *);
void ofproto_unref(struct ofproto *);

static inline const struct rule_actions * rule_get_actions(const struct rule *);
static inline bool rule_is_table_miss(const struct rule *);
Expand Down
35 changes: 31 additions & 4 deletions ofproto/ofproto.c
View file
Expand Up @@ -545,6 +545,7 @@ ofproto_create(const char *datapath_name, const char *datapath_type,

ovs_mutex_init(&ofproto->vl_mff_map.mutex);
cmap_init(&ofproto->vl_mff_map.cmap);
ovs_refcount_init(&ofproto->ref_count);

error = ofproto->ofproto_class->construct(ofproto);
if (error) {
Expand Down Expand Up @@ -1738,8 +1739,7 @@ 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);
ofproto_unref(p);
}

/* Destroys the datapath with the respective 'name' and 'type'. With the Linux
Expand Down Expand Up @@ -2928,6 +2928,7 @@ ofproto_rule_destroy__(struct rule *rule)
cls_rule_destroy(CONST_CAST(struct cls_rule *, &rule->cr));
rule_actions_destroy(rule_get_actions(rule));
ovs_mutex_destroy(&rule->mutex);
ofproto_unref(rule->ofproto);
rule->ofproto->ofproto_class->rule_dealloc(rule);
}

Expand Down Expand Up @@ -2979,6 +2980,28 @@ ofproto_rule_unref(struct rule *rule)
}
}

void ofproto_ref(struct ofproto *ofproto)
{
if (ofproto) {
ovs_refcount_ref(&ofproto->ref_count);
}
}

bool ofproto_try_ref(struct ofproto *ofproto)
{
if (ofproto) {
return ovs_refcount_try_ref_rcu(&ofproto->ref_count);
}
return false;
}

void ofproto_unref(struct ofproto *ofproto)
{
if (ofproto && ovs_refcount_unref_relaxed(&ofproto->ref_count) == 1) {
ovsrcu_postpone(ofproto_destroy_defer__, ofproto);
}
}

static void
remove_rule_rcu__(struct rule *rule)
OVS_REQUIRES(ofproto_mutex)
Expand Down Expand Up @@ -3068,6 +3091,7 @@ group_destroy_cb(struct ofgroup *group)
&group->props));
ofputil_bucket_list_destroy(CONST_CAST(struct ovs_list *,
&group->buckets));
ofproto_unref(group->ofproto);
group->ofproto->ofproto_class->group_dealloc(group);
}

Expand Down Expand Up @@ -5266,6 +5290,7 @@ ofproto_rule_create(struct ofproto *ofproto, struct cls_rule *cr,

/* Initialize base state. */
*CONST_CAST(struct ofproto **, &rule->ofproto) = ofproto;
ofproto_ref(ofproto);
cls_rule_move(CONST_CAST(struct cls_rule *, &rule->cr), cr);
ovs_refcount_init(&rule->ref_count);

Expand Down Expand Up @@ -6214,7 +6239,7 @@ handle_flow_mod__(struct ofproto *ofproto, const struct ofputil_flow_mod *fm,
error = ofproto_flow_mod_start(ofproto, &ofm);
if (!error) {
ofproto_bump_tables_version(ofproto);
error = ofproto_flow_mod_finish(ofproto, &ofm, req);
error = ofproto_flow_mod_finish(ofproto, &ofm, req);
ofmonitor_flush(ofproto->connmgr);
}
ovs_mutex_unlock(&ofproto_mutex);
Expand Down Expand Up @@ -7331,6 +7356,7 @@ init_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm,
}

*CONST_CAST(struct ofproto **, &(*ofgroup)->ofproto) = ofproto;
ofproto_ref(ofproto);
*CONST_CAST(uint32_t *, &((*ofgroup)->group_id)) = gm->group_id;
*CONST_CAST(enum ofp11_group_type *, &(*ofgroup)->type) = gm->type;
*CONST_CAST(long long int *, &((*ofgroup)->created)) = now;
Expand Down Expand Up @@ -7363,6 +7389,7 @@ 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);
ofproto_unref(ofproto);
}
return error;
}
Expand Down Expand Up @@ -8199,7 +8226,7 @@ do_bundle_commit(struct ofconn *ofconn, uint32_t id, uint16_t flags)
/* Send error referring to the original message. */
ofconn_send_error(ofconn, be->msg, error);
error = OFPERR_OFPBFC_MSG_FAILED;

/* 2. Revert. Undo all the changes made above. */
LIST_FOR_EACH_REVERSE_CONTINUE(be, node, &bundle->msg_list) {
if (be->type == OFPTYPE_FLOW_MOD) {
Expand Down

0 comments on commit 8478676

Please sign in to comment.