<p>neels has uploaded this change for <strong>review</strong>.</p><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/15660">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">add osmo_fsm_inst_watch()<br><br>I discovered an osmo-msc use-after-free crash from an invalid message, caused<br>by this pattern:<br><br> void event_action()<br> {<br> osmo_fsm_inst_dispatch(foo, FOO_EVENT, NULL);<br> osmo_fsm_inst_dispatch(bar, BAR_EVENT, NULL);<br> }<br><br>Usually, FOO_EVENT takes successful action, and afterwards we also notify bar<br>of another event. However, in this particular case FOO_EVENT caused failure,<br>and the immediate error handling directly terminated and deallocated bar.<br>In such cases, dispatching BAR_EVENT causes a use-after-free; this constituted<br>a DoS vector just from sending messages that fail to validate to osmo-msc.<br><br>Instead, introduce this pattern for accessing FSM instances after<br>failure-critical actions, which watches out for a given osmo_fsm_inst's<br>deallocation:<br><br> void event_action()<br> {<br> struct osmo_fsm_inst_watcher watch_bar;<br><br> osmo_fsm_inst_watch(&watch_bar, bar);<br> osmo_fsm_inst_dispatch(foo, FOO_EVENT, NULL);<br> osmo_fsm_inst_unwatch(&watch_bar);<br><br> if (watch_bar.exists)<br> osmo_fsm_inst_dispatch(bar, BAR_EVENT, NULL);<br> }<br><br>Implementation: at first I had thought of a simple lookup whether bar still is<br>listed in the bar->fsm list of osmo_fsm_inst instances. That worked well, but<br>theoretically, after a deallocation, another FSM may have been allocated,<br>possibly at the exact same memory address. This chance is slim, but<br>nevertheless quite possible. The only fully safe way is to explicitly watch an<br>instance.<br><br>Test: incorporate FSM instance watchers in fsm_dealloc_test.c, with<br>OSMO_ASSERTs verifying that the watchers reflect exactly whether an object is<br>still allocated. Though the test's expected output does not print anything when<br>the osmo_fsm_inst_watchers reflect the expected values, I did verify that the<br>test catches bugs when introduced deliberately.<br><br>Related: Iaa8e3da2969ebb4c78bff11d0d59f01b10f341d7 (osmo-msc),<br> I790d2172c7c67610915cb74b0766fb18f2795b29 (osmo-mgw)<br>Change-Id: I4d8306488506c60b4c2fc1c4cb3ac04654db9c43<br>---<br>M include/osmocom/core/fsm.h<br>M src/fsm.c<br>M tests/fsm/fsm_dealloc_test.c<br>3 files changed, 114 insertions(+), 1 deletion(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/60/15660/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/include/osmocom/core/fsm.h b/include/osmocom/core/fsm.h</span><br><span>index 1701c45..2487124 100644</span><br><span>--- a/include/osmocom/core/fsm.h</span><br><span>+++ b/include/osmocom/core/fsm.h</span><br><span>@@ -116,6 +116,8 @@</span><br><span> struct llist_head child;</span><br><span> /*! Indicator whether osmo_fsm_inst_term() was already invoked on this instance. */</span><br><span> bool terminating;</span><br><span style="color: hsl(120, 100%, 40%);">+ /*! See osmo_fsm_inst_watch(). */</span><br><span style="color: hsl(120, 100%, 40%);">+ struct llist_head watchers;</span><br><span> } proc;</span><br><span> };</span><br><span> </span><br><span>@@ -324,4 +326,12 @@</span><br><span> void *data,</span><br><span> const char *file, int line);</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+struct osmo_fsm_inst_watcher {</span><br><span style="color: hsl(120, 100%, 40%);">+ struct llist_head entry;</span><br><span style="color: hsl(120, 100%, 40%);">+ bool exists;</span><br><span style="color: hsl(120, 100%, 40%);">+};</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+void osmo_fsm_inst_watch(struct osmo_fsm_inst_watcher *watcher, struct osmo_fsm_inst *fi);</span><br><span style="color: hsl(120, 100%, 40%);">+void osmo_fsm_inst_unwatch(struct osmo_fsm_inst_watcher *watcher);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> /*! @} */</span><br><span>diff --git a/src/fsm.c b/src/fsm.c</span><br><span>index c886351..03f22cb 100644</span><br><span>--- a/src/fsm.c</span><br><span>+++ b/src/fsm.c</span><br><span>@@ -418,6 +418,7 @@</span><br><span> </span><br><span> INIT_LLIST_HEAD(&fi->proc.children);</span><br><span> INIT_LLIST_HEAD(&fi->proc.child);</span><br><span style="color: hsl(120, 100%, 40%);">+ INIT_LLIST_HEAD(&fi->proc.watchers);</span><br><span> llist_add(&fi->list, &fsm->instances);</span><br><span> </span><br><span> LOGPFSM(fi, "Allocated\n");</span><br><span>@@ -502,6 +503,15 @@</span><br><span> */</span><br><span> void osmo_fsm_inst_free(struct osmo_fsm_inst *fi)</span><br><span> {</span><br><span style="color: hsl(120, 100%, 40%);">+ struct osmo_fsm_inst_watcher *watcher;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ /* Notify all watchers that this is deallocating. */</span><br><span style="color: hsl(120, 100%, 40%);">+ llist_for_each_entry(watcher, &fi->proc.watchers, entry) {</span><br><span style="color: hsl(120, 100%, 40%);">+ watcher->exists = false;</span><br><span style="color: hsl(120, 100%, 40%);">+ /* There is no need to llist_del(), setting exists = false already signals that the llist's head no</span><br><span style="color: hsl(120, 100%, 40%);">+ * longer exists, and the watcher->entry should be considered to be floating alone. */</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> osmo_timer_del(&fi->timer);</span><br><span> llist_del(&fi->list);</span><br><span> </span><br><span>@@ -972,4 +982,68 @@</span><br><span> { 0, NULL }</span><br><span> };</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+/* Monitor a specific osmo_fsm_inst instance to detect whether it has terminated.</span><br><span style="color: hsl(120, 100%, 40%);">+ * For example, if you would like to do this:</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * osmo_fsm_inst_dispatch(foo, FOO_EVENT, NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+ * osmo_fsm_inst_dispatch(bar, BAR_EVENT, NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * If FSM instances foo and bar are linked in a way that the FOO_EVENT may cause bar to terminate, either directly or</span><br><span style="color: hsl(120, 100%, 40%);">+ * via various other FSM instances that dispatch events ("dominoes"), then dispatching BAR_EVENT may cause a</span><br><span style="color: hsl(120, 100%, 40%);">+ * use-after-free failure.</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * To prevent this and detect a deallocation of bar, one could look up whether bar is still listed as an instance in</span><br><span style="color: hsl(120, 100%, 40%);">+ * bar->fsm->instances. But, even if that exact pointer has been terminated and deallocated, a new FSM instance might</span><br><span style="color: hsl(120, 100%, 40%);">+ * have been in turn allocated after that, coincidentally at exactly the same memory address.</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * The only way to safely determine whether a particular FSM instance has deallocated is to put attach this handle on</span><br><span style="color: hsl(120, 100%, 40%);">+ * it. When the instance deallocates, it will write false to watcher->exists.</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * Do not forget to unwatch: it is required to call osmo_fsm_inst_unwatch(watcher) if the instance has not yet</span><br><span style="color: hsl(120, 100%, 40%);">+ * deallocated; if it has deallocated, calling osmo_fsm_inst_unwatch() is optional (has no effect).</span><br><span style="color: hsl(120, 100%, 40%);">+ * Above dispatch of BAR_EVENT can be safeguarded like this:</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * struct osmo_fsm_inst_watcher watch_bar;</span><br><span style="color: hsl(120, 100%, 40%);">+ * osmo_fsm_inst_watch(&watch_bar, bar);</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * osmo_fsm_inst_dispatch(foo, FOO_EVENT, NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+ * if (watch_bar.exists)</span><br><span style="color: hsl(120, 100%, 40%);">+ * osmo_fsm_inst_dispatch(bar, BAR_EVENT, NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * osmo_fsm_inst_unwatch(&watch_bar);</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * watch_bar.exists will be set to false just before the instance is actually being deallocated.</span><br><span style="color: hsl(120, 100%, 40%);">+ * Even if watch_bar.exists is found to be true, bar may already be busy terminating, but not yet freed (see</span><br><span style="color: hsl(120, 100%, 40%);">+ * osmo_fsm_term_safely()). To also avoid dispatching events to FSM instances that are terminating, use:</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * if (watch_bar.exists && !bar->proc.terminating)</span><br><span style="color: hsl(120, 100%, 40%);">+ * osmo_fsm_inst_dispatch(bar, BAR_EVENT, NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * \param[in,out] watcher The watcher instance to use for watching fi.</span><br><span style="color: hsl(120, 100%, 40%);">+ * \param[in] fi The FSM instance to watch.</span><br><span style="color: hsl(120, 100%, 40%);">+ */</span><br><span style="color: hsl(120, 100%, 40%);">+void osmo_fsm_inst_watch(struct osmo_fsm_inst_watcher *watcher, struct osmo_fsm_inst *fi)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_fsm_inst_unwatch(watcher);</span><br><span style="color: hsl(120, 100%, 40%);">+ watcher->exists = (fi != NULL);</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!watcher->exists)</span><br><span style="color: hsl(120, 100%, 40%);">+ return;</span><br><span style="color: hsl(120, 100%, 40%);">+ llist_add(&watcher->entry, &fi->proc.watchers);</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+/* Remove an osmo_fsm_inst_watcher from the fi it was watching, see osmo_fsm_inst_watch().</span><br><span style="color: hsl(120, 100%, 40%);">+ * It is safe to call osmo_fsm_inst_unwatch() any number of times on the same watcher.</span><br><span style="color: hsl(120, 100%, 40%);">+ * The watcher->exists flag retains its value when calling osmo_fsm_inst_unwatch().</span><br><span style="color: hsl(120, 100%, 40%);">+ * \param[in] watcher A watcher instance on which osmo_fsm_inst_watch() has been called earlier.</span><br><span style="color: hsl(120, 100%, 40%);">+ */</span><br><span style="color: hsl(120, 100%, 40%);">+void osmo_fsm_inst_unwatch(struct osmo_fsm_inst_watcher *watcher)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ /* Safeguard against calling osmo_fsm_inst_unwatch() more than once. */</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!watcher->entry.prev)</span><br><span style="color: hsl(120, 100%, 40%);">+ return;</span><br><span style="color: hsl(120, 100%, 40%);">+ if (watcher->exists)</span><br><span style="color: hsl(120, 100%, 40%);">+ llist_del(&watcher->entry);</span><br><span style="color: hsl(120, 100%, 40%);">+ watcher->entry = (struct llist_head){};</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> /*! @} */</span><br><span>diff --git a/tests/fsm/fsm_dealloc_test.c b/tests/fsm/fsm_dealloc_test.c</span><br><span>index ce49205..c8ad029 100644</span><br><span>--- a/tests/fsm/fsm_dealloc_test.c</span><br><span>+++ b/tests/fsm/fsm_dealloc_test.c</span><br><span>@@ -39,6 +39,7 @@</span><br><span> </span><br><span> struct scene {</span><br><span> struct obj *o[scene_size];</span><br><span style="color: hsl(120, 100%, 40%);">+ struct osmo_fsm_inst_watcher watcher[scene_size];</span><br><span> </span><br><span> /* The use count is actually just to help tracking what functions have not exited yet */</span><br><span> struct osmo_use_count use_count;</span><br><span>@@ -292,6 +293,7 @@</span><br><span> </span><br><span> static struct scene *scene_alloc()</span><br><span> {</span><br><span style="color: hsl(120, 100%, 40%);">+ int i;</span><br><span> struct scene *s = talloc_zero(ctx, struct scene);</span><br><span> s->use_count.talloc_object = s;</span><br><span> s->use_count.use_cb = use_cb;</span><br><span>@@ -317,6 +319,14 @@</span><br><span> obj_set_other(s->o[branch1], s->o[other]);</span><br><span> obj_set_other(s->o[twig1a], s->o[root]);</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+ for (i = 0; i < ARRAY_SIZE(s->o); i++) {</span><br><span style="color: hsl(120, 100%, 40%);">+ struct obj *o = s->o[i];</span><br><span style="color: hsl(120, 100%, 40%);">+ struct osmo_fsm_inst_watcher *watcher = &s->watcher[i];</span><br><span style="color: hsl(120, 100%, 40%);">+ if (o)</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_fsm_inst_watch(watcher, o->fi);</span><br><span style="color: hsl(120, 100%, 40%);">+ else</span><br><span style="color: hsl(120, 100%, 40%);">+ watcher->exists = false;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span> return s;</span><br><span> }</span><br><span> </span><br><span>@@ -325,9 +335,23 @@</span><br><span> int i;</span><br><span> int got = 0;</span><br><span> for (i = 0; i < ARRAY_SIZE(s->o); i++) {</span><br><span style="color: hsl(0, 100%, 40%);">- if (!s->o[i])</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!s->o[i]) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (s->watcher[i].exists) {</span><br><span style="color: hsl(120, 100%, 40%);">+ LOGP(DLGLOBAL, LOGL_ERROR,</span><br><span style="color: hsl(120, 100%, 40%);">+ " ERROR: obj[%d]: obj deallocated,"</span><br><span style="color: hsl(120, 100%, 40%);">+ " but osmo_fsm_inst_watcher still says exists==true\n",</span><br><span style="color: hsl(120, 100%, 40%);">+ i);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(false);</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span> continue;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span> LOGP(DLGLOBAL, LOGL_DEBUG, " %s\n", s->o[i]->fi->id);</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!s->watcher[i].exists) {</span><br><span style="color: hsl(120, 100%, 40%);">+ LOGP(DLGLOBAL, LOGL_ERROR,</span><br><span style="color: hsl(120, 100%, 40%);">+ " ERROR: obj[%d]: obj still present, but osmo_fsm_inst_watcher says exists==false\n",</span><br><span style="color: hsl(120, 100%, 40%);">+ i);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(false);</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span> got++;</span><br><span> }</span><br><span> return got;</span><br><span>@@ -337,6 +361,11 @@</span><br><span> {</span><br><span> int i;</span><br><span> for (i = 0; i < ARRAY_SIZE(s->o); i++) {</span><br><span style="color: hsl(120, 100%, 40%);">+ /* Call unwatch on both existing and already deallocated instances, all should work fine. */</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_fsm_inst_unwatch(&s->watcher[i]);</span><br><span style="color: hsl(120, 100%, 40%);">+ /* Verify that calling unwatch twice is fine */</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_fsm_inst_unwatch(&s->watcher[i]);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> if (!s->o[i])</span><br><span> continue;</span><br><span> osmo_fsm_inst_term(s->o[i]->fi, OSMO_FSM_TERM_ERROR, 0);</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.osmocom.org/c/libosmocore/+/15660">change 15660</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/libosmocore/+/15660"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: libosmocore </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I4d8306488506c60b4c2fc1c4cb3ac04654db9c43 </div>
<div style="display:none"> Gerrit-Change-Number: 15660 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>