<p>Stefan Sperling <strong>merged</strong> this change.</p><p><a href="https://gerrit.osmocom.org/9252">View Change</a></p><div style="white-space:pre-wrap">Approvals:
Jenkins Builder: Verified
Pau Espin Pedrol: Looks good to me, approved
</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">fix double-free/use-after-free of pointers in struct e1inp_line<br><br>Ensure that pointers in cloned e1inp_lines point to valid memory.<br>Some members of struct e1inp_line can simply be deep-copied.<br>Use talloc reference counting for pointers to objects which may<br>be shared between clones (driver-private state and counters).<br>Prevents double-free bugs, e.g. when multiple links referring<br>to the same line are closed.<br><br>Also, do not forget to unlink struct e1inp_line's counter group from<br>the counter list. Fixes use-after-free in rate_ctr_timer_cb() during<br>osmo-bts shutdown.<br><br>Change-Id: I9f4724b4a5a064801591e9acf4f2fd1db006d082<br>Related: OS#3011<br>Related: OS#3137<br>---<br>M src/e1_input.c<br>1 file changed, 40 insertions(+), 1 deletion(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/src/e1_input.c b/src/e1_input.c</span><br><span>index 29ba440..11949a1 100644</span><br><span>--- a/src/e1_input.c</span><br><span>+++ b/src/e1_input.c</span><br><span>@@ -394,6 +394,25 @@</span><br><span> return NULL;</span><br><span> </span><br><span> memcpy(clone, line, sizeof(struct e1inp_line));</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ if (line->name) {</span><br><span style="color: hsl(120, 100%, 40%);">+ clone->name = talloc_strdup(clone, line->name);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(clone->name);</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+ if (line->sock_path) {</span><br><span style="color: hsl(120, 100%, 40%);">+ clone->sock_path = talloc_strdup(clone, line->sock_path);</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(clone->sock_path);</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%);">+ /*</span><br><span style="color: hsl(120, 100%, 40%);">+ * Rate counters and driver data are shared between clones. These are pointers</span><br><span style="color: hsl(120, 100%, 40%);">+ * to dynamic memory so we use reference counting to avoid a double-free (see OS#3137).</span><br><span style="color: hsl(120, 100%, 40%);">+ */</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(line->rate_ctr);</span><br><span style="color: hsl(120, 100%, 40%);">+ clone->rate_ctr = talloc_reference(clone, line->rate_ctr);</span><br><span style="color: hsl(120, 100%, 40%);">+ if (line->driver_data)</span><br><span style="color: hsl(120, 100%, 40%);">+ clone->driver_data = talloc_reference(clone, line->driver_data);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> clone->refcnt = 1;</span><br><span> return clone;</span><br><span> }</span><br><span>@@ -406,8 +425,28 @@</span><br><span> void e1inp_line_put(struct e1inp_line *line)</span><br><span> {</span><br><span> line->refcnt--;</span><br><span style="color: hsl(0, 100%, 40%);">- if (line->refcnt == 0)</span><br><span style="color: hsl(120, 100%, 40%);">+ if (line->refcnt == 0) {</span><br><span style="color: hsl(120, 100%, 40%);">+ /* Remove our counter group from libosmocore's global counter</span><br><span style="color: hsl(120, 100%, 40%);">+ * list if we are freeing the last remaining talloc context.</span><br><span style="color: hsl(120, 100%, 40%);">+ * Otherwise we get a use-after-free when libosmocore's timer</span><br><span style="color: hsl(120, 100%, 40%);">+ * ticks again and attempts to update these counters (OS#3011).</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * Note that talloc internally counts "secondary" references</span><br><span style="color: hsl(120, 100%, 40%);">+ * _in addition to_ the initial allocation context, so yes,</span><br><span style="color: hsl(120, 100%, 40%);">+ * we must check for *zero* remaining secondary contexts here. */</span><br><span style="color: hsl(120, 100%, 40%);">+ if (talloc_reference_count(line->rate_ctr) == 0) {</span><br><span style="color: hsl(120, 100%, 40%);">+ rate_ctr_group_free(line->rate_ctr);</span><br><span style="color: hsl(120, 100%, 40%);">+ } else {</span><br><span style="color: hsl(120, 100%, 40%);">+ /* We are not freeing the last talloc context.</span><br><span style="color: hsl(120, 100%, 40%);">+ * Instead of calling talloc_free(), unlink this 'line' pointer</span><br><span style="color: hsl(120, 100%, 40%);">+ * which serves as one of several talloc contexts for the rate</span><br><span style="color: hsl(120, 100%, 40%);">+ * counters and driver private state. */</span><br><span style="color: hsl(120, 100%, 40%);">+ talloc_unlink(line, line->rate_ctr);</span><br><span style="color: hsl(120, 100%, 40%);">+ if (line->driver_data)</span><br><span style="color: hsl(120, 100%, 40%);">+ talloc_unlink(line, line->driver_data);</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span> talloc_free(line);</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span> }</span><br><span> </span><br><span> void</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.osmocom.org/9252">change 9252</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/9252"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: libosmo-abis </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: merged </div>
<div style="display:none"> Gerrit-Change-Id: I9f4724b4a5a064801591e9acf4f2fd1db006d082 </div>
<div style="display:none"> Gerrit-Change-Number: 9252 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: Stefan Sperling <ssperling@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Harald Welte <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: Pau Espin Pedrol <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Stefan Sperling <ssperling@sysmocom.de> </div>