neels has uploaded this change for review. (
https://gerrit.osmocom.org/c/libosmocore/+/15660
Change subject: add osmo_fsm_inst_watch()
......................................................................
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
---
M include/osmocom/core/fsm.h
M src/fsm.c
M tests/fsm/fsm_dealloc_test.c
3 files changed, 114 insertions(+), 1 deletion(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/60/15660/1
diff --git a/include/osmocom/core/fsm.h b/include/osmocom/core/fsm.h
index 1701c45..2487124 100644
--- a/include/osmocom/core/fsm.h
+++ b/include/osmocom/core/fsm.h
@@ -116,6 +116,8 @@
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 *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 c886351..03f22cb 100644
--- a/src/fsm.c
+++ b/src/fsm.c
@@ -418,6 +418,7 @@
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_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,68 @@
{ 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);
+ watcher->exists = (fi != NULL);
+ if (!watcher->exists)
+ return;
+ 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 ce49205..c8ad029 100644
--- a/tests/fsm/fsm_dealloc_test.c
+++ b/tests/fsm/fsm_dealloc_test.c
@@ -39,6 +39,7 @@
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 @@
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 @@
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 @@
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 @@
{
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);
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15660
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I4d8306488506c60b4c2fc1c4cb3ac04654db9c43
Gerrit-Change-Number: 15660
Gerrit-PatchSet: 1
Gerrit-Owner: neels <[email protected]>
Gerrit-MessageType: newchange
