[PATCH] osmo-bts[master]: fix handover: handle_ph_ra_ind(): evaluate ra_ind before msg...

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/.

Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Thu Mar 8 20:17:47 UTC 2018


Review at  https://gerrit.osmocom.org/7169

fix handover: handle_ph_ra_ind(): evaluate ra_ind before msgb_trim()

Commit c2b4c668f3510b7b0baace749c5a310959010e90
I3b989580cb38082e3fd8fc50a11fedda13991092 introduces evaluation of ra_ind
members below the msgb_trim() call that actually invalidates ra_ind.
A symptom is that it breaks detection of Handover RACH, wich always ends up
with lchan == NULL and interpreting all RACH as chan_nr == 0x88.

Fix: do all evaluation of ra_ind before the msgb_trim(), for osmo-bts-sysmo,
litecell-15 and octphy.

To guard against similar mistakes in the future, set ra_ind = NULL before the
msgb_trim() call.

Change-Id: I203021ee57f49cb963679ba8bec5943e2abb67fb
---
M src/osmo-bts-litecell15/l1_if.c
M src/osmo-bts-octphy/l1_if.c
M src/osmo-bts-sysmo/l1_if.c
3 files changed, 138 insertions(+), 117 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-bts refs/changes/69/7169/1

diff --git a/src/osmo-bts-litecell15/l1_if.c b/src/osmo-bts-litecell15/l1_if.c
index 9e122cd..ea652a1 100644
--- a/src/osmo-bts-litecell15/l1_if.c
+++ b/src/osmo-bts-litecell15/l1_if.c
@@ -997,10 +997,8 @@
 	struct gsm_bts_role_bts *btsb = bts->role;
 	struct gsm_lchan *lchan;
 	struct osmo_phsap_prim *l1sap;
-	uint32_t fn;
-	uint8_t acc_delay = 0;
-	uint16_t ra = 0, is_11bit = 0, burst_type = 0, temp = 0;
 	int rc;
+	struct ph_rach_ind_param rach_ind_param;
 
 	/* FIXME: this should be deprecated/obsoleted as it bypasses rach.busy counting */
 	if (ra_ind->measParam.fLinkQuality < btsb->min_qual_rach) {
@@ -1008,12 +1006,7 @@
 		return 0;
 	}
 
-	/* the old legacy full-bits acc_delay cannot express negative values */
-	if (ra_ind->measParam.i16BurstTiming > 0)
-		acc_delay = ra_ind->measParam.i16BurstTiming >> 2;
-
 	dump_meas_res(LOGL_DEBUG, &ra_ind->measParam);
-	burst_type = ra_ind->burstType;
 
 	if ((ra_ind->msgUnitParam.u8Size != 1) &&
 		(ra_ind->msgUnitParam.u8Size != 2)) {
@@ -1022,60 +1015,74 @@
 		return 0;
 	}
 
+	/* We need to evaluate ra_ind before below msgb_trim(), since that invalidates *ra_ind. */
+	rach_ind_param = (struct ph_rach_ind_param) {
+		/* .chan_nr set below */
+		/* .ra set below */
+		.acc_delay = 0,
+		.fn = ra_ind->u32Fn,
+		/* .is_11bit set below */
+		/* .burst_type set below */
+		.rssi = (int8_t) ra_ind->measParam.fRssi,
+		.ber10k = (unsigned int) (ra_ind->measParam.fBer * 10000.0),
+		.acc_delay_256bits = ra_ind->measParam.i16BurstTiming * 64,
+	};
+
+	lchan = l1if_hLayer_to_lchan(trx, (uint32_t)ra_ind->hLayer2);
+	if (!lchan || lchan->ts->pchan == GSM_PCHAN_CCCH ||
+	    lchan->ts->pchan == GSM_PCHAN_CCCH_SDCCH4 ||
+	    lchan->ts->pchan == GSM_PCHAN_CCCH_SDCCH4_CBCH)
+		rach_ind_param.chan_nr = 0x88;
+	else
+		rach_ind_param.chan_nr = gsm_lchan2chan_nr(lchan);
+
 	if (ra_ind->msgUnitParam.u8Size == 2) {
-		is_11bit = 1;
-		ra = ra_ind->msgUnitParam.u8Buffer[0];
+		uint16_t temp;
+		uint16_t ra = ra_ind->msgUnitParam.u8Buffer[0];
 		ra = ra << 3;
 		temp = (ra_ind->msgUnitParam.u8Buffer[1] & 0x7);
 		ra = ra | temp;
+		rach_ind_param.is_11bit = 1;
+		rach_ind_param.ra = ra;
 	} else {
-		is_11bit = 0;
-		ra = ra_ind->msgUnitParam.u8Buffer[0];
+		rach_ind_param.is_11bit = 0;
+		rach_ind_param.ra = ra_ind->msgUnitParam.u8Buffer[0];
 	}
 
-	fn = ra_ind->u32Fn;
+	/* the old legacy full-bits acc_delay cannot express negative values */
+	if (ra_ind->measParam.i16BurstTiming > 0)
+		rach_ind_param.acc_delay = ra_ind->measParam.i16BurstTiming >> 2;
+
+	/* mapping of the burst type, the values are specific to
+	 * osmo-bts-litecell15 */
+	switch (ra_ind->burstType) {
+	case GsmL1_BurstType_Access_0:
+		rach_ind_param.burst_type =
+			GSM_L1_BURST_TYPE_ACCESS_0;
+		break;
+	case GsmL1_BurstType_Access_1:
+		rach_ind_param.burst_type =
+			GSM_L1_BURST_TYPE_ACCESS_1;
+		break;
+	case GsmL1_BurstType_Access_2:
+		rach_ind_param.burst_type =
+			GSM_L1_BURST_TYPE_ACCESS_2;
+		break;
+	default:
+		rach_ind_param.burst_type =
+			GSM_L1_BURST_TYPE_NONE;
+		break;
+	}
+
+	/* msgb_trim() invalidates ra_ind, make that abundantly clear: */
+	ra_ind = NULL;
 	rc = msgb_trim(l1p_msg, sizeof(*l1sap));
 	if (rc < 0)
 		MSGB_ABORT(l1p_msg, "No room for primitive data\n");
 	l1sap = msgb_l1sap_prim(l1p_msg);
 	osmo_prim_init(&l1sap->oph, SAP_GSM_PH, PRIM_PH_RACH, PRIM_OP_INDICATION,
 		l1p_msg);
-	l1sap->u.rach_ind.ra = ra;
-	l1sap->u.rach_ind.acc_delay = acc_delay;
-	l1sap->u.rach_ind.fn = fn;
-	l1sap->u.rach_ind.is_11bit = is_11bit; /* no of bits in 11 bit RACH */
-	l1sap->u.rach_ind.rssi = (int8_t) ra_ind->measParam.fRssi;
-	l1sap->u.rach_ind.ber10k = (unsigned int) (ra_ind->measParam.fBer * 10000.0);
-	l1sap->u.rach_ind.acc_delay_256bits = ra_ind->measParam.i16BurstTiming * 64;
-
-	/* mapping of the burst type, the values are specific to
-	 * osmo-bts-litecell15 */
-	switch (burst_type) {
-	case GsmL1_BurstType_Access_0:
-		l1sap->u.rach_ind.burst_type =
-			GSM_L1_BURST_TYPE_ACCESS_0;
-		break;
-	case GsmL1_BurstType_Access_1:
-		l1sap->u.rach_ind.burst_type =
-			GSM_L1_BURST_TYPE_ACCESS_1;
-		break;
-	case GsmL1_BurstType_Access_2:
-		l1sap->u.rach_ind.burst_type =
-			GSM_L1_BURST_TYPE_ACCESS_2;
-		break;
-	default:
-		l1sap->u.rach_ind.burst_type =
-			GSM_L1_BURST_TYPE_NONE;
-		break;
-	}
-
-	lchan = l1if_hLayer_to_lchan(trx, (uint32_t)ra_ind->hLayer2);
-	if (!lchan || lchan->ts->pchan == GSM_PCHAN_CCCH ||
-	    lchan->ts->pchan == GSM_PCHAN_CCCH_SDCCH4 ||
-	    lchan->ts->pchan == GSM_PCHAN_CCCH_SDCCH4_CBCH)
-		l1sap->u.rach_ind.chan_nr = 0x88;
-	else
-		l1sap->u.rach_ind.chan_nr = gsm_lchan2chan_nr(lchan);
+	l1sap->u.rach_ind = rach_ind_param;
 
 	return l1sap_up(trx, l1sap);
 }
diff --git a/src/osmo-bts-octphy/l1_if.c b/src/osmo-bts-octphy/l1_if.c
index 700cc80..32b86a0 100644
--- a/src/osmo-bts-octphy/l1_if.c
+++ b/src/osmo-bts-octphy/l1_if.c
@@ -1168,12 +1168,10 @@
 	struct gsm_bts *bts = trx->bts;
 	struct gsm_bts_role_bts *btsb = bts_role_bts(bts);
 	struct osmo_phsap_prim *l1sap;
-	uint8_t ra, acc_delay;
 	int rc;
+	struct ph_rach_ind_param rach_ind_param;
 
 	dump_meas_res(LOGL_DEBUG, &ra_ind->MeasurementInfo);
-
-	ra = ra_ind->abyMsg[0];
 
 	if (ra_ind->ulMsgLength != 1) {
 		LOGPFN(DL1C, LOGL_ERROR, ra_ind->ulFrameNumber,
@@ -1182,33 +1180,43 @@
 		return 0;
 	}
 
+	/* We need to evaluate ra_ind before below msgb_trim(), since that invalidates *ra_ind. */
+	rach_ind_param = (struct ph_rach_ind_param) {
+		/* .chan_nr set below */
+		.ra = ra_ind->abyMsg[0],
+		/* .acc_delay set below */
+		.fn = ra_ind->ulFrameNumber,
+		.is_11bit = 0,
+		/* .burst_type remains unset */
+		.rssi = oct_meas2rssi_dBm(&ra_ind->MeasurementInfo),
+		.ber10k = oct_meas2ber10k(&ra_ind->MeasurementInfo),
+		.acc_delay_256bits = ra_ind->MeasurementInfo.sBurstTiming4x * 64,
+	};
+
+	if (ra_ind->LchId.bySubChannelNb == cOCTVC1_GSM_ID_SUB_CHANNEL_NB_ENUM_ALL &&
+	    ra_ind->LchId.bySAPI == cOCTVC1_GSM_SAPI_ENUM_RACH) {
+		rach_ind_param.chan_nr = 0x88;
+	} else {
+		struct gsm_lchan *lchan = get_lchan_by_lchid(trx, &ra_ind->LchId);
+		rach_ind_param.chan_nr = gsm_lchan2chan_nr(lchan);
+	}
+
 	/* check for under/overflow / sign */
 	if (ra_ind->MeasurementInfo.sBurstTiming < 0)
-		acc_delay = 0;
+		rach_ind_param.acc_delay = 0;
 	else
-		acc_delay = ra_ind->MeasurementInfo.sBurstTiming;
+		rach_ind_param.acc_delay = ra_ind->MeasurementInfo.sBurstTiming;
 
+	/* msgb_trim() invalidates ra_ind, make that abundantly clear: */
+	ra_ind = NULL;
 	rc = msgb_trim(l1p_msg, sizeof(*l1sap));
 	if (rc < 0)
 		MSGB_ABORT(l1p_msg, "No room for primitive\n");
 	l1sap = msgb_l1sap_prim(l1p_msg);
 	osmo_prim_init(&l1sap->oph, SAP_GSM_PH, PRIM_PH_RACH, PRIM_OP_INDICATION,
 			l1p_msg);
-	l1sap->u.rach_ind.ra = ra;
-	l1sap->u.rach_ind.acc_delay = acc_delay;
-	l1sap->u.rach_ind.fn = ra_ind->ulFrameNumber;
-	l1sap->u.rach_ind.is_11bit = 0;
-	l1sap->u.rach_ind.rssi = oct_meas2rssi_dBm(&ra_ind->MeasurementInfo);
-	l1sap->u.rach_ind.ber10k = oct_meas2ber10k(&ra_ind->MeasurementInfo);
-	l1sap->u.rach_ind.acc_delay_256bits = ra_ind->MeasurementInfo.sBurstTiming4x * 64;
+	l1sap->u.rach_ind = rach_ind_param;
 
-	if (ra_ind->LchId.bySubChannelNb == cOCTVC1_GSM_ID_SUB_CHANNEL_NB_ENUM_ALL &&
-	    ra_ind->LchId.bySAPI == cOCTVC1_GSM_SAPI_ENUM_RACH) {
-		l1sap->u.rach_ind.chan_nr = 0x88;
-	} else {
-		struct gsm_lchan *lchan = get_lchan_by_lchid(trx, &ra_ind->LchId);
-		l1sap->u.rach_ind.chan_nr = gsm_lchan2chan_nr(lchan);
-	}
 	l1sap_up(trx, l1sap);
 
 	/* return '1' to indicate l1sap_up has taken msgb ownership */
diff --git a/src/osmo-bts-sysmo/l1_if.c b/src/osmo-bts-sysmo/l1_if.c
index 46db69c..60eacab 100644
--- a/src/osmo-bts-sysmo/l1_if.c
+++ b/src/osmo-bts-sysmo/l1_if.c
@@ -987,10 +987,8 @@
 	struct gsm_bts_role_bts *btsb = bts->role;
 	struct gsm_lchan *lchan;
 	struct osmo_phsap_prim *l1sap;
-	uint32_t fn;
-	uint8_t acc_delay = 0;
-	uint16_t ra = 0, is_11bit = 0, burst_type = 0, temp = 0;
 	int rc;
+	struct ph_rach_ind_param rach_ind_param;
 
 	/* FIXME: this should be deprecated/obsoleted as it bypasses rach.busy counting */
 	if (ra_ind->measParam.fLinkQuality < btsb->min_qual_rach) {
@@ -998,12 +996,7 @@
 		return 0;
 	}
 
-	/* the old legacy full-bits acc_delay cannot express negative values */
-	if (ra_ind->measParam.i16BurstTiming > 0)
-		acc_delay = ra_ind->measParam.i16BurstTiming >> 2;
-
 	dump_meas_res(LOGL_DEBUG, &ra_ind->measParam);
-	burst_type = ra_ind->burstType;
 
 	if ((ra_ind->msgUnitParam.u8Size != 1) &&
 		(ra_ind->msgUnitParam.u8Size != 2)) {
@@ -1013,60 +1006,73 @@
 		return 0;
 	}
 
+	/* We need to evaluate ra_ind before below msgb_trim(), since that invalidates *ra_ind. */
+	rach_ind_param = (struct ph_rach_ind_param) {
+		/* .chan_nr set below */
+		/* .ra set below */
+		.acc_delay = 0,
+		.fn = ra_ind->u32Fn,
+		/* .is_11bit set below */
+		/* .burst_type set below */
+		.rssi = (int8_t) ra_ind->measParam.fRssi,
+		.ber10k = (unsigned int) (ra_ind->measParam.fBer * 10000.0),
+		.acc_delay_256bits = ra_ind->measParam.i16BurstTiming * 64,
+	};
+
+	lchan = l1if_hLayer_to_lchan(trx, ra_ind->hLayer2);
+	if (!lchan || lchan->ts->pchan == GSM_PCHAN_CCCH ||
+	    lchan->ts->pchan == GSM_PCHAN_CCCH_SDCCH4 ||
+	    lchan->ts->pchan == GSM_PCHAN_CCCH_SDCCH4_CBCH)
+		rach_ind_param.chan_nr = 0x88;
+	else
+		rach_ind_param.chan_nr = gsm_lchan2chan_nr(lchan);
+
 	if (ra_ind->msgUnitParam.u8Size == 2) {
-		is_11bit = 1;
-		ra = ra_ind->msgUnitParam.u8Buffer[0];
+		uint16_t temp;
+		uint16_t ra = ra_ind->msgUnitParam.u8Buffer[0];
 		ra = ra << 3;
 		temp = (ra_ind->msgUnitParam.u8Buffer[1] & 0x7);
 		ra = ra | temp;
+		rach_ind_param.is_11bit = 1;
+		rach_ind_param.ra = ra;
 	} else {
-		is_11bit = 0;
-		ra = ra_ind->msgUnitParam.u8Buffer[0];
+		rach_ind_param.is_11bit = 0;
+		rach_ind_param.ra = ra_ind->msgUnitParam.u8Buffer[0];
 	}
 
-	fn = ra_ind->u32Fn;
+	/* the old legacy full-bits acc_delay cannot express negative values */
+	if (ra_ind->measParam.i16BurstTiming > 0)
+		rach_ind_param.acc_delay = ra_ind->measParam.i16BurstTiming >> 2;
+
+	/* mapping of the burst type, the values are specific to osmo-bts-sysmo */
+	switch (ra_ind->burstType) {
+	case GsmL1_BurstType_Access_0:
+		rach_ind_param.burst_type =
+			GSM_L1_BURST_TYPE_ACCESS_0;
+		break;
+	case GsmL1_BurstType_Access_1:
+		rach_ind_param.burst_type =
+			GSM_L1_BURST_TYPE_ACCESS_1;
+		break;
+	case GsmL1_BurstType_Access_2:
+		rach_ind_param.burst_type =
+			GSM_L1_BURST_TYPE_ACCESS_2;
+		break;
+	default:
+		rach_ind_param.burst_type =
+			GSM_L1_BURST_TYPE_NONE;
+		break;
+	}
+
+	/* msgb_trim() invalidates ra_ind, make that abundantly clear: */
+	ra_ind = NULL;
 	rc = msgb_trim(l1p_msg, sizeof(*l1sap));
 	if (rc < 0)
 		MSGB_ABORT(l1p_msg, "No room for primitive data\n");
 	l1sap = msgb_l1sap_prim(l1p_msg);
 	osmo_prim_init(&l1sap->oph, SAP_GSM_PH, PRIM_PH_RACH, PRIM_OP_INDICATION,
 		l1p_msg);
-	l1sap->u.rach_ind.ra = ra;
-	l1sap->u.rach_ind.acc_delay = acc_delay;
-	l1sap->u.rach_ind.fn = fn;
-	l1sap->u.rach_ind.is_11bit = is_11bit;	/* no of bits in 11 bit RACH */
-	l1sap->u.rach_ind.rssi = (int8_t) ra_ind->measParam.fRssi;
-	l1sap->u.rach_ind.ber10k = (unsigned int) (ra_ind->measParam.fBer * 10000.0);
-	l1sap->u.rach_ind.acc_delay_256bits = ra_ind->measParam.i16BurstTiming * 64;
-
-	/*mapping of the burst type, the values are specific to osmo-bts-sysmo*/
-
-	switch (burst_type) {
-	case GsmL1_BurstType_Access_0:
-		l1sap->u.rach_ind.burst_type =
-			GSM_L1_BURST_TYPE_ACCESS_0;
-		break;
-	case GsmL1_BurstType_Access_1:
-		l1sap->u.rach_ind.burst_type =
-			GSM_L1_BURST_TYPE_ACCESS_1;
-		break;
-	case GsmL1_BurstType_Access_2:
-		l1sap->u.rach_ind.burst_type =
-			GSM_L1_BURST_TYPE_ACCESS_2;
-		break;
-	default:
-		l1sap->u.rach_ind.burst_type =
-			GSM_L1_BURST_TYPE_NONE;
-		break;
-	}
-
-	lchan = l1if_hLayer_to_lchan(trx, ra_ind->hLayer2);
-	if (!lchan || lchan->ts->pchan == GSM_PCHAN_CCCH ||
-	    lchan->ts->pchan == GSM_PCHAN_CCCH_SDCCH4 ||
-	    lchan->ts->pchan == GSM_PCHAN_CCCH_SDCCH4_CBCH)
-		l1sap->u.rach_ind.chan_nr = 0x88;
-	else
-		l1sap->u.rach_ind.chan_nr = gsm_lchan2chan_nr(lchan);
+	l1sap->u.rach_ind = rach_ind_param;
 
 	return l1sap_up(trx, l1sap);
 }

-- 
To view, visit https://gerrit.osmocom.org/7169
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I203021ee57f49cb963679ba8bec5943e2abb67fb
Gerrit-PatchSet: 1
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr <nhofmeyr at sysmocom.de>



More information about the gerrit-log mailing list