diff options
author | Neels Hofmeyr <neels@hofmeyr.de> | 2018-03-08 20:04:42 +0100 |
---|---|---|
committer | Neels Hofmeyr <neels@hofmeyr.de> | 2018-03-08 21:18:34 +0100 |
commit | c2045474b1b500a6cc38c00cc807978ce9e8c5e5 (patch) | |
tree | cdca7c2d8b7fbb4a961a1956d0dbddd2b08c5d9c /src | |
parent | 7eed026efc0b75608b37521147db140c06fca5b1 (diff) |
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.
Related: OS#3045
Change-Id: I203021ee57f49cb963679ba8bec5943e2abb67fb
Diffstat (limited to 'src')
-rw-r--r-- | src/osmo-bts-litecell15/l1_if.c | 83 | ||||
-rw-r--r-- | src/osmo-bts-octphy/l1_if.c | 46 | ||||
-rw-r--r-- | src/osmo-bts-sysmo/l1_if.c | 86 |
3 files changed, 118 insertions, 97 deletions
diff --git a/src/osmo-bts-litecell15/l1_if.c b/src/osmo-bts-litecell15/l1_if.c index 9e122cd8..ea652a15 100644 --- a/src/osmo-bts-litecell15/l1_if.c +++ b/src/osmo-bts-litecell15/l1_if.c @@ -997,10 +997,8 @@ static int handle_ph_ra_ind(struct lc15l1_hdl *fl1, GsmL1_PhRaInd_t *ra_ind, 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 @@ static int handle_ph_ra_ind(struct lc15l1_hdl *fl1, GsmL1_PhRaInd_t *ra_ind, 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 @@ static int handle_ph_ra_ind(struct lc15l1_hdl *fl1, GsmL1_PhRaInd_t *ra_ind, 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; - 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; + /* 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 (burst_type) { + switch (ra_ind->burstType) { case GsmL1_BurstType_Access_0: - l1sap->u.rach_ind.burst_type = + rach_ind_param.burst_type = GSM_L1_BURST_TYPE_ACCESS_0; break; case GsmL1_BurstType_Access_1: - l1sap->u.rach_ind.burst_type = + rach_ind_param.burst_type = GSM_L1_BURST_TYPE_ACCESS_1; break; case GsmL1_BurstType_Access_2: - l1sap->u.rach_ind.burst_type = + rach_ind_param.burst_type = GSM_L1_BURST_TYPE_ACCESS_2; break; default: - l1sap->u.rach_ind.burst_type = + rach_ind_param.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); + /* 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 = 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 700cc80b..32b86a0a 100644 --- a/src/osmo-bts-octphy/l1_if.c +++ b/src/osmo-bts-octphy/l1_if.c @@ -1168,13 +1168,11 @@ static int handle_ph_rach_ind(struct octphy_hdl *fl1, 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, "Rx PH-RACH.ind has lenghth %d > 1\n", ra_ind->ulMsgLength); @@ -1182,33 +1180,43 @@ static int handle_ph_rach_ind(struct octphy_hdl *fl1, 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 46db69c3..60eacab9 100644 --- a/src/osmo-bts-sysmo/l1_if.c +++ b/src/osmo-bts-sysmo/l1_if.c @@ -987,10 +987,8 @@ static int handle_ph_ra_ind(struct femtol1_hdl *fl1, GsmL1_PhRaInd_t *ra_ind, 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 @@ static int handle_ph_ra_ind(struct femtol1_hdl *fl1, GsmL1_PhRaInd_t *ra_ind, 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 @@ static int handle_ph_ra_ind(struct femtol1_hdl *fl1, GsmL1_PhRaInd_t *ra_ind, 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; - 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*/ + /* 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; - switch (burst_type) { + /* mapping of the burst type, the values are specific to osmo-bts-sysmo */ + switch (ra_ind->burstType) { case GsmL1_BurstType_Access_0: - l1sap->u.rach_ind.burst_type = + rach_ind_param.burst_type = GSM_L1_BURST_TYPE_ACCESS_0; break; case GsmL1_BurstType_Access_1: - l1sap->u.rach_ind.burst_type = + rach_ind_param.burst_type = GSM_L1_BURST_TYPE_ACCESS_1; break; case GsmL1_BurstType_Access_2: - l1sap->u.rach_ind.burst_type = + rach_ind_param.burst_type = GSM_L1_BURST_TYPE_ACCESS_2; break; default: - l1sap->u.rach_ind.burst_type = + rach_ind_param.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); + /* 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 = rach_ind_param; return l1sap_up(trx, l1sap); } |