denozord/strongswan-ext
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix deadlock in trap_manager_t during acquire.

Browse Source 
Also fixes a TOCTOU issue regarding the use of entry_t.pending.

The deadlock was caused because the rwlock was being locked while
waiting for an IKE_SA. Triggering the deadlock was a bit tricky, here
is the description by Thomas Egerer (the reporter of this issue):

"
The deadlock occurs when the following happens (in the given order):

a) an IKE_SA is built and a thread is processing the IKE_AUTH request,
   which can take a bit longer when a smartcard is involved. This
   causes the ike_sa_manager to lock a particular IKE_SA exclusively.
b) an acquire is triggered which causes the rwlock in the trap_manager
   to be read-locked, the subsequent call to
   ike_sa_manager->checkout_by_config has to wait until a) unlocks
   it's ike_sa.
c) a child_cfg contained in the peer_cfg belonging to the ike_sa
   a) has locked is routed causes the child_configs contained
   in the peer config to be locked by c) while the actual routing
   code within trap_manager tries to writelock it's rwlock.

That's about it. As soon as a) finishes authentication of the peer
and tries to find a matching child sa it will try to lock the child
configs of the peer config which is not possible since it has been
locked by c).

Thread | Resource locked                | Resource desired
-------+--------------------------------+--------------------------------
  (a)  | ike_sa in ike_sa_manager       | child_cfgs of peer_cfg
       |                                |
  (b)  | rwlock in trap-manager (read)  | ike_sa in ike_sa_manager
       |                                |
  (c)  | child_cfgs of peer_cfg         | rwlock in trap-manager (write)
"

With this patch thread (b) now does not hold the lock while waiting for
the IKE_SA. Thus (c) can get the write lock, and (a) can subsequently
lock the mutex in the peer_cfg which then finally allows (b) to checkout
the IKE_SA.
This commit is contained in:
Tobias Brunner
2011-12-23 11:07:14 +01:00
parent 5317dd6887
commit fc726f1359
1 changed files with 44 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/libcharon/sa/trap_manager.c
+44 -29
View File
@@ -1,4 +1,5 @@
/*
* Copyright (C) 2011 Tobias Brunner
* Copyright (C) 2009 Martin Willi
* Hochschule fuer Technik Rapperswil
*
@@ -74,8 +75,10 @@ typedef struct {
peer_cfg_t *peer_cfg;
/** ref to instanciated CHILD_SA */
child_sa_t *child_sa;
/** TRUE if an acquire is pending */
bool pending;
/** pending IKE_SA connecting upon acquire */
ike_sa_t *pending;
ike_sa_t *ike_sa;
} entry_t;
/**
@@ -170,10 +173,10 @@ METHOD(trap_manager_t, install, u_int32_t,
}
reqid = child_sa->get_reqid(child_sa);
entry = malloc_thing(entry_t);
entry->child_sa = child_sa;
entry->peer_cfg = peer->get_ref(peer);
entry->pending = NULL;
INIT(entry,
.child_sa = child_sa,
.peer_cfg = peer->get_ref(peer),
);
this->lock->write_lock(this->lock);
this->traps->insert_last(this->traps, entry);
@@ -263,35 +266,46 @@ METHOD(trap_manager_t, acquire, void,
if (!found)
{
DBG1(DBG_CFG, "trap not found, unable to acquire reqid %d",reqid);
this->lock->unlock(this->lock);
return;
}
else if (found->pending)
if (!cas_bool(&found->pending, FALSE, TRUE))
{
DBG1(DBG_CFG, "ignoring acquire, connection attempt pending");
this->lock->unlock(this->lock);
return;
}
peer = found->peer_cfg->get_ref(found->peer_cfg);
child = found->child_sa->get_config(found->child_sa);
child = child->get_ref(child);
reqid = found->child_sa->get_reqid(found->child_sa);
/* don't hold the lock while checking out the IKE_SA */
this->lock->unlock(this->lock);
ike_sa = charon->ike_sa_manager->checkout_by_config(
charon->ike_sa_manager, peer);
if (ike_sa->get_peer_cfg(ike_sa) == NULL)
{
ike_sa->set_peer_cfg(ike_sa, peer);
}
if (ike_sa->initiate(ike_sa, child, reqid, src, dst) != DESTROY_ME)
{
/* make sure the entry is still there */
this->lock->read_lock(this->lock);
if (this->traps->find_first(this->traps, NULL,
(void**)&found) == SUCCESS)
{
found->ike_sa = ike_sa;
}
this->lock->unlock(this->lock);
charon->ike_sa_manager->checkin(charon->ike_sa_manager, ike_sa);
}
else
{
child = found->child_sa->get_config(found->child_sa);
peer = found->peer_cfg;
ike_sa = charon->ike_sa_manager->checkout_by_config(
charon->ike_sa_manager, peer);
if (ike_sa->get_peer_cfg(ike_sa) == NULL)
{
ike_sa->set_peer_cfg(ike_sa, peer);
}
child->get_ref(child);
reqid = found->child_sa->get_reqid(found->child_sa);
if (ike_sa->initiate(ike_sa, child, reqid, src, dst) != DESTROY_ME)
{
found->pending = ike_sa;
charon->ike_sa_manager->checkin(charon->ike_sa_manager, ike_sa);
}
else
{
charon->ike_sa_manager->checkin_and_destroy(
charon->ike_sa_manager, ike_sa);
}
charon->ike_sa_manager->checkin_and_destroy(
charon->ike_sa_manager, ike_sa);
}
this->lock->unlock(this->lock);
peer->destroy(peer);
}
/**
@@ -307,7 +321,7 @@ static void complete(private_trap_manager_t *this, ike_sa_t *ike_sa,
enumerator = this->traps->create_enumerator(this->traps);
while (enumerator->enumerate(enumerator, &entry))
{
if (entry->pending != ike_sa)
if (entry->ike_sa != ike_sa)
{
continue;
}
@@ -316,7 +330,8 @@ static void complete(private_trap_manager_t *this, ike_sa_t *ike_sa,
{
continue;
}
entry->pending = NULL;
entry->ike_sa = NULL;
entry->pending = FALSE;
}
enumerator->destroy(enumerator);
this->lock->unlock(this->lock);