d/freebsd-skq
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Fix possible memory leak and several races in the IPsec policy management

Browse Source 
code.

Resurrect the state field in the struct secpolicy, it has
IPSEC_SPSTATE_ALIVE value when security policy linked in the chain,
and IPSEC_SPSTATE_DEAD value in all other cases. This field protects
from trying to unlink one security policy several times from the different
threads.

Take additional reference in the key_flush_spd() to be sure that policy
won't be freed from the different thread while we are sending SPDEXPIRE message.

Add KEY_FREESP() call to the key_unlink() to release additional reference
that we take when use key_getsp*() functions.

Differential Revision:	https://reviews.freebsd.org/D1914
Tested by:		Emeric POUPON <emeric.poupon at stormshield dot eu>
Reviewed by:	hrs
Sponsored by:	Yandex LLC
This commit is contained in:
ae 2015-02-24 10:35:07 +00:00
parent b9f1e32cfa
commit 217cb9ff26
2 changed files with 18 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
sys/netipsec/ipsec.h
View File

@ -89,6 +89,9 @@ struct secpolicy {
/* if policy == IPSEC else this value == NULL.*/
u_int refcnt; /* reference count */
u_int policy; /* policy_type per pfkeyv2.h */
u_int state;
#define IPSEC_SPSTATE_DEAD 0
#define IPSEC_SPSTATE_ALIVE 1
u_int32_t id; /* It's unique number on the system. */
/*
* lifetime handler.

16
sys/netipsec/key.c
View File

@ -1198,8 +1198,14 @@ key_unlink(struct secpolicy *sp)
SPTREE_UNLOCK_ASSERT();
SPTREE_WLOCK();
if (sp->state == IPSEC_SPSTATE_DEAD) {
SPTREE_WUNLOCK();
return;
}
sp->state = IPSEC_SPSTATE_DEAD;
TAILQ_REMOVE(&V_sptree[sp->spidx.dir], sp, chain);
SPTREE_WUNLOCK();
KEY_FREESP(&sp);
}
/*
@ -1900,6 +1906,7 @@ key_spdadd(struct socket *so, struct mbuf *m, const struct sadb_msghdr *mhp)
SPTREE_WLOCK();
TAILQ_INSERT_TAIL(&V_sptree[newsp->spidx.dir], newsp, chain);
newsp->state = IPSEC_SPSTATE_ALIVE;
SPTREE_WUNLOCK();
/* delete the entry in spacqtree */
@ -2335,6 +2342,12 @@ key_spdflush(struct socket *so, struct mbuf *m, const struct sadb_msghdr *mhp)
for (dir = 0; dir < IPSEC_DIR_MAX; dir++) {
TAILQ_CONCAT(&drainq, &V_sptree[dir], chain);
}
/*
* We need to set state to DEAD for each policy to be sure,
* that another thread won't try to unlink it.
*/
TAILQ_FOREACH(sp, &drainq, chain)
sp->state = IPSEC_SPSTATE_DEAD;
SPTREE_WUNLOCK();
sp = TAILQ_FIRST(&drainq);
while (sp != NULL) {
@ -4209,9 +4222,10 @@ restart:
now - sp->created > sp->lifetime) ||
(sp->validtime &&
now - sp->lastused > sp->validtime)) {
SP_ADDREF(sp);
SPTREE_RUNLOCK();
key_unlink(sp);
key_spdexpire(sp);
key_unlink(sp);
KEY_FREESP(&sp);
goto restart;
}