From fc757c58dc2617db30146c128e1ab6c47133910f Mon Sep 17 00:00:00 2001 From: Karsten Keil Date: Tue, 14 Oct 2014 14:06:28 +0200 Subject: [PATCH] Better refcount locking to avoid use after free Under some conditions it could be happen that on a freeing capi object a other thread still get a reference and then it use the already freed object. To avoid this, we do 2 things, we only take a refernce if get_obj() succeed, it do not longer succeed if the object is already cleaned. We also force scheduling after releaseing the lock before really freeing the object - so the waiting thread will not get a new reference. --- capi20/application.c | 16 ++++++++++++---- capi20/capi_obj.c | 38 ++++++++++++++++++++++++++++---------- capi20/daemon.c | 24 ++++++++++++++++++------ capi20/listen.c | 12 ++++++++++-- capi20/lplci.c | 33 +++++++++++++++++++++++++++------ capi20/m_capi.h | 15 +++++++++++---- capi20/ncci.c | 16 +++++++++++++--- 7 files changed, 119 insertions(+), 35 deletions(-) diff --git a/capi20/application.c b/capi20/application.c index ac13141..05b4aa9 100644 --- a/capi20/application.c +++ b/capi20/application.c @@ -103,8 +103,12 @@ int register_lController(struct mApplication *appl, struct lController *lc) eprint("%s: controller idx %d ID:%d\already registered\n", CAPIobjIDstr(&appl->cobj), i, lc->cobj.id); return -EBUSY; } - appl->lcl[i] = lc; - get_cobj(&lc->cobj); + if (get_cobj(&lc->cobj)) { + appl->lcl[i] = lc; + } else { + eprint("%s: controller idx %d cannot get controller object %s\n", CAPIobjIDstr(&appl->cobj), i, CAPIobjIDstr(&lc->cobj)); + return -EINVAL; + } return 0; } @@ -290,8 +294,12 @@ struct lController *get_lController(struct mApplication *appl, unsigned int cont wprint("%s: wrong controller id %d (max %d)\n", CAPIobjIDstr(&appl->cobj), cont, mI_ControllerCount); lc = NULL; } - if (lc) - get_cobj(&lc->cobj); + if (lc) { + if (!get_cobj(&lc->cobj)) { + wprint("%s: cannot get controller object %s\n", CAPIobjIDstr(&appl->cobj), CAPIobjIDstr(&lc->cobj)); + lc = NULL; + } + } return lc; } diff --git a/capi20/capi_obj.c b/capi20/capi_obj.c index 7340575..2032f22 100644 --- a/capi20/capi_obj.c +++ b/capi20/capi_obj.c @@ -132,6 +132,7 @@ void CAPIobj_exit(void) while (co) { cn = co->nextD; eprint("%s: uid=%i refcnt %d in dangling list - freeing now\n", CAPIobjIDstr(co), co->uid, co->refcnt); + co->cleaned = 1; cobj_free(co); co = cn; } @@ -357,9 +358,12 @@ struct mCAPIobj *__get_cobj(struct mCAPIobj *co, const char *file, int lineno) struct mCAPIobj *get_cobj(struct mCAPIobj *co) #endif { + struct mCAPIobj *p; + if (co) { - if (co->parent) { - pthread_rwlock_wrlock(&co->parent->lock); + p = co->parent; + if (p) { + pthread_rwlock_wrlock(&p->lock); } else { if (co->type != Cot_Root) { /* has no parent */ cobj_err("%s: parent not assigned\n", CAPIobjIDstr(co)); @@ -367,12 +371,17 @@ struct mCAPIobj *get_cobj(struct mCAPIobj *co) } pthread_mutex_lock(&rootLock); } - if (co->freeing) - cobj_err("Currently freeing %s refcnt: %d\n", CAPIobjIDstr(co), co->refcnt); - co->refcnt++; - coref_dbg("%s\n", CAPIobjIDstr(co)); - if (co->parent) - pthread_rwlock_unlock(&co->parent->lock); + if (co->cleaned) { + cobj_warn("%s: pending free detected - do not get object\n", CAPIobjIDstr(co)); + co = NULL; + } else { + if (co->freeing) + cobj_err("Currently freeing %s refcnt: %d\n", CAPIobjIDstr(co), co->refcnt); + co->refcnt++; + coref_dbg("%s\n", CAPIobjIDstr(co)); + } + if (p) + pthread_rwlock_unlock(&p->lock); else pthread_mutex_unlock(&rootLock); } else @@ -403,6 +412,10 @@ int put_cobj(struct mCAPIobj *co) ret = co->refcnt; if (co->cleaned && co->refcnt <= 0) { /* last ref */ pthread_rwlock_unlock(&p->lock); + /* OK not perfect but scheduling here should prevent us from access of freed memory, if a other thread + still pending on the lock - a cleaned object is not longer listed and do not return success in get_obj() + so getting a new reference should not happen after this point */ + sched_yield(); cobj_free(co); } else { pthread_rwlock_unlock(&p->lock); @@ -447,8 +460,13 @@ struct mCAPIobj *get_next_cobj(struct mCAPIobj *parent, struct mCAPIobj *cur) else next = parent->listhead; if (next) { - next->refcnt++; - coref_dbg("%s: next %s\n", CAPIobjIDstr(cur), CAPIobjIDstr(next)); + if (next->cleaned) { + cobj_warn("%s: pending free detected - do not get next\n", CAPIobjIDstr(next)); + next = NULL; + } else { + next->refcnt++; + coref_dbg("%s: next %s\n", CAPIobjIDstr(cur), CAPIobjIDstr(next)); + } } pthread_rwlock_unlock(&parent->lock); if (parent->freeing) diff --git a/capi20/daemon.c b/capi20/daemon.c index b574e3f..c52ddd5 100644 --- a/capi20/daemon.c +++ b/capi20/daemon.c @@ -1141,6 +1141,16 @@ int OpenBInstance(struct BInstance *bi, struct lPLCI *lp) return ret; } + if (get_cobj(&lp->cobj)) { + bi->lp = lp; + } else { + bi->lp = NULL; + ret = -EINVAL; + wprint("Cannot get logical controller object\n"); + close(sk); + return ret; + } + switch (lp->btype) { case BType_Direct: bi->from_down = recvBdirect; @@ -1160,6 +1170,8 @@ int OpenBInstance(struct BInstance *bi, struct lPLCI *lp) eprint("Error unnkown BType %d\n", lp->btype); dprint(MIDEBUG_CONTROLLER, "close socket bi[%d]->fd %d\n", bi->nr, sk); close(sk); + bi->lp = NULL; + put_cobj(&lp->cobj); return -EINVAL; } bi->type = lp->btype; @@ -1181,11 +1193,11 @@ int OpenBInstance(struct BInstance *bi, struct lPLCI *lp) bi->from_down = dummy_btrans; bi->from_up = dummy_btrans; bi->type = BType_None; + bi->lp = NULL; + put_cobj(&lp->cobj); } else { bi->closed = 0; bi->fd = sk; - bi->lp = lp; - get_cobj(&lp->cobj); bi->org_rx_min = MISDN_CTRL_RX_SIZE_IGNORE; bi->rx_min = MISDN_CTRL_RX_SIZE_IGNORE; bi->org_rx_max = MISDN_CTRL_RX_SIZE_IGNORE; @@ -1243,8 +1255,8 @@ int OpenBInstance(struct BInstance *bi, struct lPLCI *lp) dprint(MIDEBUG_CONTROLLER, "close socket bi[%d]->fd %d\n", bi->nr, sk); close(sk); bi->fd = -1; - put_cobj(&lp->cobj); bi->lp = NULL; + put_cobj(&lp->cobj); } else { dprint(MIDEBUG_CONTROLLER, "Controller%d: Bchannel %d socket %d added to poll idx %d\n", bi->pc->profile.ncontroller, bi->nr, sk, ret); @@ -1260,8 +1272,8 @@ int OpenBInstance(struct BInstance *bi, struct lPLCI *lp) dprint(MIDEBUG_CONTROLLER, "close socket bi[%d]->fd %d\n", bi->nr, sk); close(sk); bi->fd = -1; - put_cobj(&lp->cobj); bi->lp = NULL; + put_cobj(&lp->cobj); } else { ret = 0; bi->UpId = 0; @@ -1274,8 +1286,8 @@ int OpenBInstance(struct BInstance *bi, struct lPLCI *lp) dprint(MIDEBUG_CONTROLLER, "close socket bi[%d]->fd %d\n", bi->nr, sk); close(sk); bi->fd = -1; - put_cobj(&lp->cobj); bi->lp = NULL; + put_cobj(&lp->cobj); return ret; } else { bi->pfd[2].fd = bi->tty; @@ -1287,8 +1299,8 @@ int OpenBInstance(struct BInstance *bi, struct lPLCI *lp) dprint(MIDEBUG_CONTROLLER, "close socket bi[%d]->fd %d\n", bi->nr, sk); close(sk); bi->fd = -1; - put_cobj(&lp->cobj); bi->lp = NULL; + put_cobj(&lp->cobj); } else { ret = 0; bi->UpId = 0; diff --git a/capi20/listen.c b/capi20/listen.c index 8388db9..850e954 100644 --- a/capi20/listen.c +++ b/capi20/listen.c @@ -128,14 +128,19 @@ struct lController *addlController(struct mApplication *app, struct pController lc = calloc(1, sizeof(*lc)); if (lc) { lc->cobj.id2 = app->cobj.id2; + if (!get_cobj(&app->cobj)) { + eprint("Cannot get application object\n"); + free(lc); + return NULL; + } ret = init_cobj_registered(&lc->cobj, &pc->cobjLC, Cot_lController, 0x0000ff); if (ret) { eprint("Controller%d: Application %d - cannot init\n", pc->profile.ncontroller, app->cobj.id2); + put_cobj(&app->cobj); free(lc); lc = NULL; } else { lc->Appl = app; - get_cobj(&app->cobj); lc->listen_m.fsm = &listen_fsm; lc->listen_m.state = ST_LISTEN_L_0; lc->listen_m.debug = MIDEBUG_CONTROLLER & mI_debug_mask; @@ -146,10 +151,13 @@ struct lController *addlController(struct mApplication *app, struct pController lc->CIPmask2 = 0; ret = register_lController(app, lc); if (ret) { + lc->Appl = NULL; put_cobj(&app->cobj); eprint("Controller%d: - cannot register LC on Application %d - %s\n", pc->profile.ncontroller, app->cobj.id2, strerror(-ret)); - free(lc); + lc->cobj.cleaned = 1; + delist_cobj(&lc->cobj); + put_cobj(&lc->cobj); lc = NULL; } } diff --git a/capi20/lplci.c b/capi20/lplci.c index b185ff1..0370c41 100644 --- a/capi20/lplci.c +++ b/capi20/lplci.c @@ -1747,10 +1747,24 @@ int lPLCICreate(struct lPLCI **lpp, struct lController *lc, struct mPLCI *plci, return ret; } lp->cipmask = cipmask; - lp->lc = lc; - get_cobj(&lc->cobj); - lp->Appl = lc->Appl; - get_cobj(&lc->Appl->cobj); + if (get_cobj(&lc->cobj)) { + lp->lc = lc; + } else { + wprint("%s: Cannot get lController object\n", CAPIobjIDstr(&lc->cobj)); + lp->cobj.cleaned = 1; + delist_cobj(&lp->cobj); + put_cobj(&lp->cobj); /* will cleanup and free too */ + return -EINVAL; + } + if (get_cobj(&lc->Appl->cobj)) { + lp->Appl = lc->Appl; + } else { + wprint("%s: Cannot get application object\n", CAPIobjIDstr(&lc->Appl->cobj)); + lp->cobj.cleaned = 1; + delist_cobj(&lp->cobj); + put_cobj(&lp->cobj); /* will cleanup and free too */ + return -EINVAL; + } if (plci->pc->profile.goptions & 0x0008) { /* DTMF */ lp->l1dtmf = 1; @@ -1762,8 +1776,15 @@ int lPLCICreate(struct lPLCI **lpp, struct lController *lc, struct mPLCI *plci, lp->plci_m.printdebug = lPLCI_debug; lp->chid.nr = MI_CHAN_NONE; lp->autohangup = 1; - init_timer(&lp->atimer, mICAPItimer_base, lp, atimer_timeout); - get_cobj(&lp->cobj); /* timer ref */ + if (get_cobj(&lp->cobj)) { /* timer ref */ + init_timer(&lp->atimer, mICAPItimer_base, lp, atimer_timeout); + } else { + wprint("%s: Cannot get lplci object for timer ref\n", CAPIobjIDstr(&lp->cobj)); + lp->cobj.cleaned = 1; + delist_cobj(&lp->cobj); + put_cobj(&lp->cobj); /* will cleanup and free too */ + return -EINVAL; + } *lpp = lp; return 0; } diff --git a/capi20/m_capi.h b/capi20/m_capi.h index faa88ce..147c4ef 100644 --- a/capi20/m_capi.h +++ b/capi20/m_capi.h @@ -133,19 +133,26 @@ struct mCAPIobj { #endif }; +#if __GNUC_PREREQ (3,4) +# define __WUR __attribute__ ((__warn_unused_result__)) +#else +# define __WUR +#endif + + #ifdef MISDN_CAPI_REFCOUNT_DEBUG -struct mCAPIobj *__get_cobj(struct mCAPIobj *, const char *, int); +struct mCAPIobj *__get_cobj(struct mCAPIobj *, const char *, int) __WUR; int __put_cobj(struct mCAPIobj *, const char *, int); -struct mCAPIobj *__get_next_cobj(struct mCAPIobj *, struct mCAPIobj *, const char *, int); +struct mCAPIobj *__get_next_cobj(struct mCAPIobj *, struct mCAPIobj *, const char *, int) __WUR; int __delist_cobj(struct mCAPIobj *, const char *, int); #define get_cobj(co) __get_cobj(co, __FILE__, __LINE__) #define put_cobj(co) __put_cobj(co, __FILE__, __LINE__) #define get_next_cobj(pa, co) __get_next_cobj(pa, co, __FILE__, __LINE__) #define delist_cobj(co) __delist_cobj(co, __FILE__, __LINE__) #else -struct mCAPIobj *get_cobj(struct mCAPIobj *); +struct mCAPIobj *get_cobj(struct mCAPIobj *) __WUR; int put_cobj(struct mCAPIobj *); -struct mCAPIobj *get_next_cobj(struct mCAPIobj *, struct mCAPIobj *); +struct mCAPIobj *get_next_cobj(struct mCAPIobj *, struct mCAPIobj *) __WUR; int delist_cobj(struct mCAPIobj *); #endif diff --git a/capi20/ncci.c b/capi20/ncci.c index 8e005c1..6629205 100644 --- a/capi20/ncci.c +++ b/capi20/ncci.c @@ -640,6 +640,7 @@ struct mNCCI *ncciCreate(struct lPLCI *lp) { struct mNCCI *nc; int err; + struct mApplication *appl = NULL; nc = calloc(1, sizeof(*nc)); if (!nc) { @@ -647,6 +648,15 @@ struct mNCCI *ncciCreate(struct lPLCI *lp) return NULL; } nc->cobj.id2 = lp->cobj.id2; + if (lp->Appl) { + if (get_cobj(&lp->Appl->cobj)) { + appl = lp->Appl; + } else { + wprint("Cannot get application object\n"); + free(nc); + return NULL; + } + } err = init_cobj_registered(&nc->cobj, &lp->cobj, Cot_NCCI, 0x01ffff); if (!err) { dprint(MIDEBUG_NCCI, "NCCI%06x: will be created now\n", nc->cobj.id); @@ -654,9 +664,7 @@ struct mNCCI *ncciCreate(struct lPLCI *lp) nc->ncci_m.debug = MIDEBUG_NCCI & mI_debug_mask; nc->ncci_m.userdata = nc; nc->ncci_m.printdebug = ncci_debug; - nc->appl = lp->Appl; - if (nc->appl) - get_cobj(&nc->appl->cobj); + nc->appl = appl; nc->BIlink = lp->BIlink; nc->window = lp->Appl->MaxB3Blk; pthread_mutex_init(&nc->lock, NULL); @@ -710,6 +718,8 @@ struct mNCCI *ncciCreate(struct lPLCI *lp) nc->osize = 256; dprint(MIDEBUG_NCCI, "%s: created\n", CAPIobjIDstr(&nc->cobj)); } else { + if (appl) + put_cobj(&appl->cobj); free(nc); nc = NULL; }