aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorNeels Hofmeyr <neels@hofmeyr.de>2019-09-19 05:36:05 +0200
committerNeels Hofmeyr <neels@hofmeyr.de>2019-10-04 17:58:31 +0200
commit442a67e36946e035e0c2aa221d546939e91d9872 (patch)
tree972b756e0f717e96a209f27dfdc3eff25da21112
parent4ad3cb1044df8538f8014e8e2ac6d1dc0dc0c451 (diff)
add osmo_fsm_inst_watch()
I discovered an osmo-msc use-after-free crash from an invalid message, caused by this pattern: void event_action() { osmo_fsm_inst_dispatch(foo, FOO_EVENT, NULL); osmo_fsm_inst_dispatch(bar, BAR_EVENT, NULL); } Usually, FOO_EVENT takes successful action, and afterwards we also notify bar of another event. However, in this particular case FOO_EVENT caused failure, and the immediate error handling directly terminated and deallocated bar. In such cases, dispatching BAR_EVENT causes a use-after-free; this constituted a DoS vector just from sending messages that fail to validate to osmo-msc. Instead, introduce this pattern for accessing FSM instances after failure-critical actions, which watches out for a given osmo_fsm_inst's deallocation: void event_action() { struct osmo_fsm_inst_watcher watch_bar; osmo_fsm_inst_watch(&watch_bar, bar); osmo_fsm_inst_dispatch(foo, FOO_EVENT, NULL); osmo_fsm_inst_unwatch(&watch_bar); if (watch_bar.exists) osmo_fsm_inst_dispatch(bar, BAR_EVENT, NULL); } Implementation: at first I had thought of a simple lookup whether bar still is listed in the bar->fsm list of osmo_fsm_inst instances. That worked well, but theoretically, after a deallocation, another FSM may have been allocated, possibly at the exact same memory address. This chance is slim, but nevertheless quite possible. The only fully safe way is to explicitly watch an instance. Test: incorporate FSM instance watchers in fsm_dealloc_test.c, with OSMO_ASSERTs verifying that the watchers reflect exactly whether an object is still allocated. Though the test's expected output does not print anything when the osmo_fsm_inst_watchers reflect the expected values, I did verify that the test catches bugs when introduced deliberately. Related: Iaa8e3da2969ebb4c78bff11d0d59f01b10f341d7 (osmo-msc), I790d2172c7c67610915cb74b0766fb18f2795b29 (osmo-mgw) Change-Id: I4d8306488506c60b4c2fc1c4cb3ac04654db9c43
-rw-r--r--include/osmocom/core/fsm.h10
-rw-r--r--src/fsm.c76
-rw-r--r--tests/fsm/fsm_dealloc_test.c31
3 files changed, 116 insertions, 1 deletions
diff --git a/include/osmocom/core/fsm.h b/include/osmocom/core/fsm.h
index 1701c45e..24871243 100644
--- a/include/osmocom/core/fsm.h
+++ b/include/osmocom/core/fsm.h
@@ -116,6 +116,8 @@ struct osmo_fsm_inst {
struct llist_head child;
/*! Indicator whether osmo_fsm_inst_term() was already invoked on this instance. */
bool terminating;
+ /*! See osmo_fsm_inst_watch(). */
+ struct llist_head watchers;
} proc;
};
@@ -324,4 +326,12 @@ void _osmo_fsm_inst_term_children(struct osmo_fsm_inst *fi,
void *data,
const char *file, int line);
+struct osmo_fsm_inst_watcher {
+ struct llist_head entry;
+ bool exists;
+};
+
+void osmo_fsm_inst_watch(struct osmo_fsm_inst_watcher *watcher, struct osmo_fsm_inst *fi);
+void osmo_fsm_inst_unwatch(struct osmo_fsm_inst_watcher *watcher);
+
/*! @} */
diff --git a/src/fsm.c b/src/fsm.c
index c8863513..4d6cdd65 100644
--- a/src/fsm.c
+++ b/src/fsm.c
@@ -418,6 +418,7 @@ struct osmo_fsm_inst *osmo_fsm_inst_alloc(struct osmo_fsm *fsm, void *ctx, void
INIT_LLIST_HEAD(&fi->proc.children);
INIT_LLIST_HEAD(&fi->proc.child);
+ INIT_LLIST_HEAD(&fi->proc.watchers);
llist_add(&fi->list, &fsm->instances);
LOGPFSM(fi, "Allocated\n");
@@ -502,6 +503,15 @@ void osmo_fsm_inst_change_parent(struct osmo_fsm_inst *fi,
*/
void osmo_fsm_inst_free(struct osmo_fsm_inst *fi)
{
+ struct osmo_fsm_inst_watcher *watcher;
+
+ /* Notify all watchers that this is deallocating. */
+ llist_for_each_entry(watcher, &fi->proc.watchers, entry) {
+ watcher->exists = false;
+ /* There is no need to llist_del(), setting exists = false already signals that the llist's head no
+ * longer exists, and the watcher->entry should be considered to be floating alone. */
+ }
+
osmo_timer_del(&fi->timer);
llist_del(&fi->list);
@@ -972,4 +982,70 @@ const struct value_string osmo_fsm_term_cause_names[] = {
{ 0, NULL }
};
+/* Monitor a specific osmo_fsm_inst instance to detect whether it has terminated.
+ * For example, if you would like to do this:
+ *
+ * osmo_fsm_inst_dispatch(foo, FOO_EVENT, NULL);
+ * osmo_fsm_inst_dispatch(bar, BAR_EVENT, NULL);
+ *
+ * If FSM instances foo and bar are linked in a way that the FOO_EVENT may cause bar to terminate, either directly or
+ * via various other FSM instances that dispatch events ("dominoes"), then dispatching BAR_EVENT may cause a
+ * use-after-free failure.
+ *
+ * To prevent this and detect a deallocation of bar, one could look up whether bar is still listed as an instance in
+ * bar->fsm->instances. But, even if that exact pointer has been terminated and deallocated, a new FSM instance might
+ * have been in turn allocated after that, coincidentally at exactly the same memory address.
+ *
+ * The only way to safely determine whether a particular FSM instance has deallocated is to put attach this handle on
+ * it. When the instance deallocates, it will write false to watcher->exists.
+ *
+ * Do not forget to unwatch: it is required to call osmo_fsm_inst_unwatch(watcher) if the instance has not yet
+ * deallocated; if it has deallocated, calling osmo_fsm_inst_unwatch() is optional (has no effect).
+ * Above dispatch of BAR_EVENT can be safeguarded like this:
+ *
+ * struct osmo_fsm_inst_watcher watch_bar;
+ * osmo_fsm_inst_watch(&watch_bar, bar);
+ *
+ * osmo_fsm_inst_dispatch(foo, FOO_EVENT, NULL);
+ * if (watch_bar.exists)
+ * osmo_fsm_inst_dispatch(bar, BAR_EVENT, NULL);
+ *
+ * osmo_fsm_inst_unwatch(&watch_bar);
+ *
+ * watch_bar.exists will be set to false just before the instance is actually being deallocated.
+ * Even if watch_bar.exists is found to be true, bar may already be busy terminating, but not yet freed (see
+ * osmo_fsm_term_safely()). To also avoid dispatching events to FSM instances that are terminating, use:
+ *
+ * if (watch_bar.exists && !bar->proc.terminating)
+ * osmo_fsm_inst_dispatch(bar, BAR_EVENT, NULL);
+ *
+ * \param[in,out] watcher The watcher instance to use for watching fi.
+ * \param[in] fi The FSM instance to watch.
+ */
+void osmo_fsm_inst_watch(struct osmo_fsm_inst_watcher *watcher, struct osmo_fsm_inst *fi)
+{
+ osmo_fsm_inst_unwatch(watcher);
+ if (!fi) {
+ watcher->exists = false;
+ return;
+ }
+ watcher->exists = true;
+ llist_add(&watcher->entry, &fi->proc.watchers);
+}
+
+/* Remove an osmo_fsm_inst_watcher from the fi it was watching, see osmo_fsm_inst_watch().
+ * It is safe to call osmo_fsm_inst_unwatch() any number of times on the same watcher.
+ * The watcher->exists flag retains its value when calling osmo_fsm_inst_unwatch().
+ * \param[in] watcher A watcher instance on which osmo_fsm_inst_watch() has been called earlier.
+ */
+void osmo_fsm_inst_unwatch(struct osmo_fsm_inst_watcher *watcher)
+{
+ /* Safeguard against calling osmo_fsm_inst_unwatch() more than once. */
+ if (!watcher->entry.prev)
+ return;
+ if (watcher->exists)
+ llist_del(&watcher->entry);
+ watcher->entry = (struct llist_head){};
+}
+
/*! @} */
diff --git a/tests/fsm/fsm_dealloc_test.c b/tests/fsm/fsm_dealloc_test.c
index ce492056..c8ad0296 100644
--- a/tests/fsm/fsm_dealloc_test.c
+++ b/tests/fsm/fsm_dealloc_test.c
@@ -39,6 +39,7 @@ enum objname {
struct scene {
struct obj *o[scene_size];
+ struct osmo_fsm_inst_watcher watcher[scene_size];
/* The use count is actually just to help tracking what functions have not exited yet */
struct osmo_use_count use_count;
@@ -292,6 +293,7 @@ void obj_set_other(struct obj *a, struct obj *b)
static struct scene *scene_alloc()
{
+ int i;
struct scene *s = talloc_zero(ctx, struct scene);
s->use_count.talloc_object = s;
s->use_count.use_cb = use_cb;
@@ -317,6 +319,14 @@ static struct scene *scene_alloc()
obj_set_other(s->o[branch1], s->o[other]);
obj_set_other(s->o[twig1a], s->o[root]);
+ for (i = 0; i < ARRAY_SIZE(s->o); i++) {
+ struct obj *o = s->o[i];
+ struct osmo_fsm_inst_watcher *watcher = &s->watcher[i];
+ if (o)
+ osmo_fsm_inst_watch(watcher, o->fi);
+ else
+ watcher->exists = false;
+ }
return s;
}
@@ -325,9 +335,23 @@ static int scene_dump(struct scene *s)
int i;
int got = 0;
for (i = 0; i < ARRAY_SIZE(s->o); i++) {
- if (!s->o[i])
+ if (!s->o[i]) {
+ if (s->watcher[i].exists) {
+ LOGP(DLGLOBAL, LOGL_ERROR,
+ " ERROR: obj[%d]: obj deallocated,"
+ " but osmo_fsm_inst_watcher still says exists==true\n",
+ i);
+ OSMO_ASSERT(false);
+ }
continue;
+ }
LOGP(DLGLOBAL, LOGL_DEBUG, " %s\n", s->o[i]->fi->id);
+ if (!s->watcher[i].exists) {
+ LOGP(DLGLOBAL, LOGL_ERROR,
+ " ERROR: obj[%d]: obj still present, but osmo_fsm_inst_watcher says exists==false\n",
+ i);
+ OSMO_ASSERT(false);
+ }
got++;
}
return got;
@@ -337,6 +361,11 @@ static void scene_clean(struct scene *s)
{
int i;
for (i = 0; i < ARRAY_SIZE(s->o); i++) {
+ /* Call unwatch on both existing and already deallocated instances, all should work fine. */
+ osmo_fsm_inst_unwatch(&s->watcher[i]);
+ /* Verify that calling unwatch twice is fine */
+ osmo_fsm_inst_unwatch(&s->watcher[i]);
+
if (!s->o[i])
continue;
osmo_fsm_inst_term(s->o[i]->fi, OSMO_FSM_TERM_ERROR, 0);