<p>pespin has uploaded this change for <strong>review</strong>.</p><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/24812">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">pdch: Fix heap-use-after-free in pdch->ulc<br><br>In existing previous code, pdch->ulc would be freed in<br>gprs_rlcmac_pdch::free_resources() when it became disabled as per PCUIF<br>info_ind (for instance, when a DYN TS is switched PDCH->SDCCH8).<br>However, pdch->ulc was so far only allocated during pdch_init, which is<br>only called during bts_alloc() time.<br>Hence, after first info_ind disabling it, if it became again enabled<br>(again by info_ind re-enabling it after SDCCH8 was not longer in use),<br>the pdch->ulc would be used again but it would point to freed memory.<br><br>Let's rearrange how/when resources are freed to make it more logical.<br>With this patch, pdch internal resources are freed upon ->disable(), and<br>re-allocated upon ->enable().<br><br>Change-Id: Id51f5f6a54ac9f24b784c17bc360ac38f5726fc7<br>---<br>M src/pcu_l1_if.cpp<br>M src/pdch.cpp<br>M src/pdch.h<br>M tests/tbf/TbfTest.cpp<br>M tests/ulc/PdchUlcTest.cpp<br>5 files changed, 26 insertions(+), 17 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/12/24812/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/src/pcu_l1_if.cpp b/src/pcu_l1_if.cpp</span><br><span>index 2373f60..5aa8849 100644</span><br><span>--- a/src/pcu_l1_if.cpp</span><br><span>+++ b/src/pcu_l1_if.cpp</span><br><span>@@ -650,7 +650,7 @@</span><br><span> for (trx_nr = 0; trx_nr < ARRAY_SIZE(bts->trx); trx_nr++) {</span><br><span> bts->trx[trx_nr].arfcn = info_ind->trx[trx_nr].arfcn;</span><br><span> for (ts_nr = 0; ts_nr < ARRAY_SIZE(bts->trx[0].pdch); ts_nr++)</span><br><span style="color: hsl(0, 100%, 40%);">- bts->trx[trx_nr].pdch[ts_nr].free_resources();</span><br><span style="color: hsl(120, 100%, 40%);">+ bts->trx[trx_nr].pdch[ts_nr].disable();</span><br><span> }</span><br><span> gprs_bssgp_destroy(bts);</span><br><span> exit(0);</span><br><span>@@ -818,7 +818,6 @@</span><br><span> } else {</span><br><span> if (pdch->is_enabled()) {</span><br><span> pcu_tx_act_req(bts, pdch, 0);</span><br><span style="color: hsl(0, 100%, 40%);">- pdch->free_resources();</span><br><span> pdch->disable();</span><br><span> }</span><br><span> }</span><br><span>diff --git a/src/pdch.cpp b/src/pdch.cpp</span><br><span>index 9321384..fbfc474 100644</span><br><span>--- a/src/pdch.cpp</span><br><span>+++ b/src/pdch.cpp</span><br><span>@@ -139,19 +139,24 @@</span><br><span> /* Initialize the PTCCH/D message (Packet Timing Advance Control Channel) */</span><br><span> memset(pdch->ptcch_msg, PTCCH_TAI_FREE, PTCCH_TAI_NUM);</span><br><span> memset(pdch->ptcch_msg + PTCCH_TAI_NUM, PTCCH_PADDING, 7);</span><br><span style="color: hsl(0, 100%, 40%);">- pdch->ulc = pdch_ulc_alloc(pdch, trx->bts);</span><br><span> }</span><br><span> </span><br><span> void gprs_rlcmac_pdch::enable()</span><br><span> {</span><br><span style="color: hsl(0, 100%, 40%);">- /* TODO: Check if there are still allocated resources.. */</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(m_is_enabled == 0);</span><br><span> INIT_LLIST_HEAD(&paging_list);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(!this->ulc);</span><br><span style="color: hsl(120, 100%, 40%);">+ this->ulc = pdch_ulc_alloc(this, trx->bts);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> m_is_enabled = 1;</span><br><span> }</span><br><span> </span><br><span> void gprs_rlcmac_pdch::disable()</span><br><span> {</span><br><span style="color: hsl(0, 100%, 40%);">- /* TODO.. kick free_resources once we know the TRX/TS we are on */</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(m_is_enabled == 1);</span><br><span style="color: hsl(120, 100%, 40%);">+ this->free_resources();</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> m_is_enabled = 0;</span><br><span> }</span><br><span> </span><br><span>@@ -159,10 +164,6 @@</span><br><span> {</span><br><span> struct gprs_rlcmac_paging *pag;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- /* we are not enabled. there should be no resources */</span><br><span style="color: hsl(0, 100%, 40%);">- if (!is_enabled())</span><br><span style="color: hsl(0, 100%, 40%);">- return;</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span> /* kick all TBF on slot */</span><br><span> pdch_free_all_tbf(this);</span><br><span> </span><br><span>@@ -171,6 +172,7 @@</span><br><span> talloc_free(pag);</span><br><span> </span><br><span> talloc_free(this->ulc);</span><br><span style="color: hsl(120, 100%, 40%);">+ this->ulc = NULL;</span><br><span> }</span><br><span> </span><br><span> struct gprs_rlcmac_paging *gprs_rlcmac_pdch::dequeue_paging()</span><br><span>diff --git a/src/pdch.h b/src/pdch.h</span><br><span>index cfa0195..b8f2911 100644</span><br><span>--- a/src/pdch.h</span><br><span>+++ b/src/pdch.h</span><br><span>@@ -55,8 +55,6 @@</span><br><span> </span><br><span> bool add_paging(uint8_t chan_needed, const struct osmo_mobile_identity *mi);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- void free_resources();</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span> bool is_enabled() const;</span><br><span> </span><br><span> void enable();</span><br><span>@@ -145,6 +143,8 @@</span><br><span> enum gprs_rlcmac_tbf_direction dir);</span><br><span> gprs_rlcmac_tbf *tbf_by_tfi(uint8_t tfi,</span><br><span> enum gprs_rlcmac_tbf_direction dir);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ void free_resources();</span><br><span> #endif</span><br><span> </span><br><span> uint8_t m_num_tbfs[2];</span><br><span>diff --git a/tests/tbf/TbfTest.cpp b/tests/tbf/TbfTest.cpp</span><br><span>index 2c2c7ae..5065e90 100644</span><br><span>--- a/tests/tbf/TbfTest.cpp</span><br><span>+++ b/tests/tbf/TbfTest.cpp</span><br><span>@@ -2282,7 +2282,7 @@</span><br><span> the_pcu->alloc_algorithm = alloc_algorithm_b;</span><br><span> bts->trx[0].pdch[2].enable();</span><br><span> bts->trx[0].pdch[3].enable();</span><br><span style="color: hsl(0, 100%, 40%);">- bts->trx[0].pdch[4].enable();</span><br><span style="color: hsl(120, 100%, 40%);">+ /* bts->trx[0].pdch[4].enable(); Already enabled during setup_bts() */</span><br><span> bts->trx[0].pdch[5].enable();</span><br><span> </span><br><span> gprs_bssgp_init(bts, 4234, 4234, 1, 1, false, 0, 0, 0);</span><br><span>@@ -2327,7 +2327,7 @@</span><br><span> the_pcu->alloc_algorithm = alloc_algorithm_b;</span><br><span> bts->trx[0].pdch[2].enable();</span><br><span> bts->trx[0].pdch[3].enable();</span><br><span style="color: hsl(0, 100%, 40%);">- bts->trx[0].pdch[4].enable();</span><br><span style="color: hsl(120, 100%, 40%);">+ /* bts->trx[0].pdch[4].enable(); Already enabled during setup_bts()) */</span><br><span> bts->trx[0].pdch[5].enable();</span><br><span> </span><br><span> gprs_bssgp_init(bts, 5234, 5234, 1, 1, false, 0, 0, 0);</span><br><span>diff --git a/tests/ulc/PdchUlcTest.cpp b/tests/ulc/PdchUlcTest.cpp</span><br><span>index c980e33..3547bf6 100644</span><br><span>--- a/tests/ulc/PdchUlcTest.cpp</span><br><span>+++ b/tests/ulc/PdchUlcTest.cpp</span><br><span>@@ -49,11 +49,19 @@</span><br><span> }</span><br><span> }</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+static struct gprs_rlcmac_bts *setup_new_bts(void)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ struct gprs_rlcmac_bts *bts = bts_alloc(the_pcu, 0);</span><br><span style="color: hsl(120, 100%, 40%);">+ struct gprs_rlcmac_pdch *pdch = &bts->trx[0].pdch[0];</span><br><span style="color: hsl(120, 100%, 40%);">+ pdch->enable();</span><br><span style="color: hsl(120, 100%, 40%);">+ return bts;</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> static void test_reserve_multiple()</span><br><span> {</span><br><span> printf("=== start: %s ===\n", __FUNCTION__);</span><br><span> const uint32_t fn = 20;</span><br><span style="color: hsl(0, 100%, 40%);">- struct gprs_rlcmac_bts *bts = bts_alloc(the_pcu, 0);</span><br><span style="color: hsl(120, 100%, 40%);">+ struct gprs_rlcmac_bts *bts = setup_new_bts();</span><br><span> struct gprs_rlcmac_pdch *pdch = &bts->trx[0].pdch[0];</span><br><span> struct gprs_rlcmac_tbf *tbf1 = (struct gprs_rlcmac_tbf*)0x1234; /*Dummy pointer */</span><br><span> struct gprs_rlcmac_tbf *tbf2 = (struct gprs_rlcmac_tbf*)0x5678; /*Dummy pointer */</span><br><span>@@ -172,7 +180,7 @@</span><br><span> </span><br><span> the_pcu->alloc_algorithm = _alloc_algorithm_dummy;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- struct gprs_rlcmac_bts *bts = bts_alloc(the_pcu, 0);</span><br><span style="color: hsl(120, 100%, 40%);">+ struct gprs_rlcmac_bts *bts = setup_new_bts();</span><br><span> struct GprsMs *ms = ms_alloc(bts, 0x12345678);</span><br><span> struct gprs_rlcmac_tbf *tbf1 = tbf_alloc_dl_tbf(bts, ms, 0, true);</span><br><span> struct gprs_rlcmac_pdch *pdch = &tbf1->trx->pdch[0];</span><br><span>@@ -215,7 +223,7 @@</span><br><span> static void test_next_free_fn_sba()</span><br><span> {</span><br><span> printf("=== start: %s ===\n", __FUNCTION__);</span><br><span style="color: hsl(0, 100%, 40%);">- struct gprs_rlcmac_bts *bts = bts_alloc(the_pcu, 0);</span><br><span style="color: hsl(120, 100%, 40%);">+ struct gprs_rlcmac_bts *bts = setup_new_bts();</span><br><span> struct gprs_rlcmac_pdch *pdch = &bts->trx[0].pdch[0];</span><br><span> struct gprs_rlcmac_sba *sba1, *sba2, *sba3, *sba4;</span><br><span> </span><br><span>@@ -239,7 +247,7 @@</span><br><span> static void test_next_free_fn_rrbp()</span><br><span> {</span><br><span> printf("=== start: %s ===\n", __FUNCTION__);</span><br><span style="color: hsl(0, 100%, 40%);">- struct gprs_rlcmac_bts *bts = bts_alloc(the_pcu, 0);</span><br><span style="color: hsl(120, 100%, 40%);">+ struct gprs_rlcmac_bts *bts = setup_new_bts();</span><br><span> struct gprs_rlcmac_pdch *pdch = &bts->trx[0].pdch[0];</span><br><span> struct gprs_rlcmac_sba *sba1;</span><br><span> uint32_t poll_fn, curr_fn;</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-pcu/+/24812">change 24812</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/osmo-pcu/+/24812"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-pcu </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: Id51f5f6a54ac9f24b784c17bc360ac38f5726fc7 </div>
<div style="display:none"> Gerrit-Change-Number: 24812 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>