Change in osmo-pcu[master]: pdch: Fix heap-use-after-free in pdch->ulc

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

pespin gerrit-no-reply at lists.osmocom.org
Sun Jul 4 18:07:14 UTC 2021


pespin has submitted this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/24812 )

Change subject: pdch: Fix heap-use-after-free in pdch->ulc
......................................................................

pdch: Fix heap-use-after-free in pdch->ulc

In existing previous code, pdch->ulc would be freed in
gprs_rlcmac_pdch::free_resources() when  it became disabled as per PCUIF
info_ind (for instance, when a DYN TS is switched PDCH->SDCCH8).
However, pdch->ulc was so far only allocated during pdch_init, which is
only called during bts_alloc() time.
Hence, after first info_ind disabling it, if it became again enabled
(again by info_ind re-enabling it after SDCCH8 was not longer in use),
the pdch->ulc would be used again but it would point to freed memory.

Let's rearrange how/when resources are freed to make it more logical.
With this patch, pdch internal resources are freed upon ->disable(), and
re-allocated upon ->enable().

Change-Id: Id51f5f6a54ac9f24b784c17bc360ac38f5726fc7
---
M src/osmobts_sock.c
M src/pcu_l1_if.cpp
M src/pdch.cpp
M src/pdch.h
M tests/tbf/TbfTest.cpp
M tests/ulc/PdchUlcTest.cpp
6 files changed, 36 insertions(+), 19 deletions(-)

Approvals:
  Jenkins Builder: Verified
  fixeria: Looks good to me, but someone else must approve
  laforge: Looks good to me, but someone else must approve
  pespin: Looks good to me, approved



diff --git a/src/osmobts_sock.c b/src/osmobts_sock.c
index 5c6415f..91b62a0 100644
--- a/src/osmobts_sock.c
+++ b/src/osmobts_sock.c
@@ -121,7 +121,8 @@
 			}
 #endif
 			for (ts = 0; ts < 8; ts++)
-				pdch_disable(&bts->trx[trx].pdch[ts]);
+				if (pdch_is_enabled(&bts->trx[trx].pdch[ts]))
+					pdch_disable(&bts->trx[trx].pdch[ts]);
 	/* FIXME: NOT ALL RESOURCES are freed in this case... inconsistent with the other code. Share the code with pcu_l1if.c
 	for the reset. */
 			bts_trx_free_all_tbf(&bts->trx[trx]);
diff --git a/src/pcu_l1_if.cpp b/src/pcu_l1_if.cpp
index 2373f60..5aa8849 100644
--- a/src/pcu_l1_if.cpp
+++ b/src/pcu_l1_if.cpp
@@ -650,7 +650,7 @@
 		for (trx_nr = 0; trx_nr < ARRAY_SIZE(bts->trx); trx_nr++) {
 			bts->trx[trx_nr].arfcn = info_ind->trx[trx_nr].arfcn;
 			for (ts_nr = 0; ts_nr < ARRAY_SIZE(bts->trx[0].pdch); ts_nr++)
-				bts->trx[trx_nr].pdch[ts_nr].free_resources();
+				bts->trx[trx_nr].pdch[ts_nr].disable();
 		}
 		gprs_bssgp_destroy(bts);
 		exit(0);
@@ -818,7 +818,6 @@
 			} else {
 				if (pdch->is_enabled()) {
 					pcu_tx_act_req(bts, pdch, 0);
-					pdch->free_resources();
 					pdch->disable();
 				}
 			}
diff --git a/src/pdch.cpp b/src/pdch.cpp
index 9321384..2ec40ce 100644
--- a/src/pdch.cpp
+++ b/src/pdch.cpp
@@ -139,19 +139,24 @@
 	/*  Initialize the PTCCH/D message (Packet Timing Advance Control Channel) */
 	memset(pdch->ptcch_msg, PTCCH_TAI_FREE, PTCCH_TAI_NUM);
 	memset(pdch->ptcch_msg + PTCCH_TAI_NUM, PTCCH_PADDING, 7);
-	pdch->ulc = pdch_ulc_alloc(pdch, trx->bts);
 }
 
 void gprs_rlcmac_pdch::enable()
 {
-	/* TODO: Check if there are still allocated resources.. */
+	OSMO_ASSERT(m_is_enabled == 0);
 	INIT_LLIST_HEAD(&paging_list);
+
+	OSMO_ASSERT(!this->ulc);
+	this->ulc = pdch_ulc_alloc(this, trx->bts);
+
 	m_is_enabled = 1;
 }
 
 void gprs_rlcmac_pdch::disable()
 {
-	/* TODO.. kick free_resources once we know the TRX/TS we are on */
+	OSMO_ASSERT(m_is_enabled == 1);
+	this->free_resources();
+
 	m_is_enabled = 0;
 }
 
@@ -159,10 +164,6 @@
 {
 	struct gprs_rlcmac_paging *pag;
 
-	/* we are not enabled. there should be no resources */
-	if (!is_enabled())
-		return;
-
 	/* kick all TBF on slot */
 	pdch_free_all_tbf(this);
 
@@ -171,6 +172,7 @@
 		talloc_free(pag);
 
 	talloc_free(this->ulc);
+	this->ulc = NULL;
 }
 
 struct gprs_rlcmac_paging *gprs_rlcmac_pdch::dequeue_paging()
@@ -1174,6 +1176,12 @@
 	}
 }
 
-void pdch_disable(struct gprs_rlcmac_pdch *pdch) {
+void pdch_disable(struct gprs_rlcmac_pdch *pdch)
+{
 	pdch->disable();
 }
+
+bool pdch_is_enabled(const struct gprs_rlcmac_pdch *pdch)
+{
+	return pdch->is_enabled();
+}
diff --git a/src/pdch.h b/src/pdch.h
index cfa0195..00f0b9d 100644
--- a/src/pdch.h
+++ b/src/pdch.h
@@ -55,8 +55,6 @@
 
 	bool add_paging(uint8_t chan_needed, const struct osmo_mobile_identity *mi);
 
-	void free_resources();
-
 	bool is_enabled() const;
 
 	void enable();
@@ -145,6 +143,8 @@
 		enum gprs_rlcmac_tbf_direction dir);
 	gprs_rlcmac_tbf *tbf_by_tfi(uint8_t tfi,
 		enum gprs_rlcmac_tbf_direction dir);
+
+	void free_resources();
 #endif
 
 	uint8_t m_num_tbfs[2];
@@ -191,6 +191,7 @@
 void pdch_init(struct gprs_rlcmac_pdch *pdch, struct gprs_rlcmac_trx *trx, uint8_t ts_nr);
 void pdch_free_all_tbf(struct gprs_rlcmac_pdch *pdch);
 void pdch_disable(struct gprs_rlcmac_pdch *pdch);
+bool pdch_is_enabled(const struct gprs_rlcmac_pdch *pdch);
 #ifdef __cplusplus
 }
 #endif
diff --git a/tests/tbf/TbfTest.cpp b/tests/tbf/TbfTest.cpp
index 2c2c7ae..5065e90 100644
--- a/tests/tbf/TbfTest.cpp
+++ b/tests/tbf/TbfTest.cpp
@@ -2282,7 +2282,7 @@
 	the_pcu->alloc_algorithm = alloc_algorithm_b;
 	bts->trx[0].pdch[2].enable();
 	bts->trx[0].pdch[3].enable();
-	bts->trx[0].pdch[4].enable();
+	/* bts->trx[0].pdch[4].enable(); Already enabled during setup_bts() */
 	bts->trx[0].pdch[5].enable();
 
 	gprs_bssgp_init(bts, 4234, 4234, 1, 1, false, 0, 0, 0);
@@ -2327,7 +2327,7 @@
 	the_pcu->alloc_algorithm = alloc_algorithm_b;
 	bts->trx[0].pdch[2].enable();
 	bts->trx[0].pdch[3].enable();
-	bts->trx[0].pdch[4].enable();
+	/* bts->trx[0].pdch[4].enable(); Already enabled during setup_bts()) */
 	bts->trx[0].pdch[5].enable();
 
 	gprs_bssgp_init(bts, 5234, 5234, 1, 1, false, 0, 0, 0);
diff --git a/tests/ulc/PdchUlcTest.cpp b/tests/ulc/PdchUlcTest.cpp
index c980e33..3547bf6 100644
--- a/tests/ulc/PdchUlcTest.cpp
+++ b/tests/ulc/PdchUlcTest.cpp
@@ -49,11 +49,19 @@
 	}
 }
 
+static struct gprs_rlcmac_bts *setup_new_bts(void)
+{
+	struct gprs_rlcmac_bts *bts = bts_alloc(the_pcu, 0);
+	struct gprs_rlcmac_pdch *pdch = &bts->trx[0].pdch[0];
+	pdch->enable();
+	return bts;
+}
+
 static void test_reserve_multiple()
 {
 	printf("=== start: %s ===\n", __FUNCTION__);
 	const uint32_t fn = 20;
-	struct gprs_rlcmac_bts *bts = bts_alloc(the_pcu, 0);
+	struct gprs_rlcmac_bts *bts = setup_new_bts();
 	struct gprs_rlcmac_pdch *pdch = &bts->trx[0].pdch[0];
 	struct gprs_rlcmac_tbf *tbf1 = (struct gprs_rlcmac_tbf*)0x1234; /*Dummy pointer */
 	struct gprs_rlcmac_tbf *tbf2 = (struct gprs_rlcmac_tbf*)0x5678; /*Dummy pointer */
@@ -172,7 +180,7 @@
 
 	the_pcu->alloc_algorithm = _alloc_algorithm_dummy;
 
-	struct gprs_rlcmac_bts *bts = bts_alloc(the_pcu, 0);
+	struct gprs_rlcmac_bts *bts = setup_new_bts();
 	struct GprsMs *ms = ms_alloc(bts, 0x12345678);
 	struct gprs_rlcmac_tbf *tbf1 = tbf_alloc_dl_tbf(bts, ms, 0, true);
 	struct gprs_rlcmac_pdch *pdch = &tbf1->trx->pdch[0];
@@ -215,7 +223,7 @@
 static void test_next_free_fn_sba()
 {
 	printf("=== start: %s ===\n", __FUNCTION__);
-	struct gprs_rlcmac_bts *bts = bts_alloc(the_pcu, 0);
+	struct gprs_rlcmac_bts *bts = setup_new_bts();
 	struct gprs_rlcmac_pdch *pdch = &bts->trx[0].pdch[0];
 	struct gprs_rlcmac_sba *sba1, *sba2, *sba3, *sba4;
 
@@ -239,7 +247,7 @@
 static void test_next_free_fn_rrbp()
 {
 	printf("=== start: %s ===\n", __FUNCTION__);
-	struct gprs_rlcmac_bts *bts = bts_alloc(the_pcu, 0);
+	struct gprs_rlcmac_bts *bts = setup_new_bts();
 	struct gprs_rlcmac_pdch *pdch = &bts->trx[0].pdch[0];
 	struct gprs_rlcmac_sba *sba1;
 	uint32_t poll_fn, curr_fn;

-- 
To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/24812
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: Id51f5f6a54ac9f24b784c17bc360ac38f5726fc7
Gerrit-Change-Number: 24812
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy at sysmocom.de>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210704/18ee99d6/attachment.htm>


More information about the gerrit-log mailing list