Change in osmo-bts[master]: rsl: refactor handling of RSL_IE_MR_CONFIG

[laforge]

laforge has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/18680 )

Change subject: rsl: refactor handling of RSL_IE_MR_CONFIG
......................................................................

rsl: refactor handling of RSL_IE_MR_CONFIG

  - get rid of gsm_lchan::mr_bts_lv, it's never used anyway,
  - check IE length in amr_parse_mr_conf() before parsing,
  - check return code of amr_parse_mr_conf().

Change-Id: Ibfd5845ea429945b352dd14421e86562998d65ca
---
M include/osmo-bts/gsm_data_shared.h
M src/common/amr.c
M src/common/rsl.c
3 files changed, 20 insertions(+), 21 deletions(-)

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



diff --git a/include/osmo-bts/gsm_data_shared.h b/include/osmo-bts/gsm_data_shared.h
index f010dbc..dc7d39f 100644
--- a/include/osmo-bts/gsm_data_shared.h
+++ b/include/osmo-bts/gsm_data_shared.h
@@ -157,9 +157,6 @@
 		uint8_t key[MAX_A5_KEY_LEN];
 	} encr;
 
-	/* AMR bits */
-	uint8_t mr_bts_lv[7];
-
 	struct {
 		uint32_t bound_ip;
 		uint32_t connect_ip;
diff --git a/src/common/amr.c b/src/common/amr.c
index 05d1aaa..837757f 100644
--- a/src/common/amr.c
+++ b/src/common/amr.c
@@ -78,13 +78,16 @@
 int amr_parse_mr_conf(struct amr_multirate_conf *amr_mrc,
 		      const uint8_t *mr_conf, unsigned int len)
 {
-	uint8_t mr_version = mr_conf[0] >> 5;
 	uint8_t num_codecs = 0;
 	int i, j = 0;
 
-	if (mr_version != 1) {
-		LOGP(DRSL, LOGL_ERROR, "AMR Multirate Version %u unknown\n",
-			mr_version);
+	if (len < 2) {
+		LOGP(DRSL, LOGL_ERROR, "AMR Multirate IE is too short (%u)\n", len);
+		goto ret_einval;
+	}
+
+	if ((mr_conf[0] >> 5) != 1) {
+		LOGP(DRSL, LOGL_ERROR, "AMR Multirate Version %u unknown\n", (mr_conf[0] >> 5));
 		goto ret_einval;
 	}
 
diff --git a/src/common/rsl.c b/src/common/rsl.c
index 41dd243..f057a89 100644
--- a/src/common/rsl.c
+++ b/src/common/rsl.c
@@ -1209,17 +1209,16 @@
 	}
 	/* 9.3.52 MultiRate Configuration */
 	if (TLVP_PRESENT(&tp, RSL_IE_MR_CONFIG)) {
-		if (TLVP_LEN(&tp, RSL_IE_MR_CONFIG) > sizeof(lchan->mr_bts_lv) - 1) {
+		rc = amr_parse_mr_conf(&lchan->tch.amr_mr,
+				       TLVP_VAL(&tp, RSL_IE_MR_CONFIG),
+				       TLVP_LEN(&tp, RSL_IE_MR_CONFIG));
+		if (rc < 0) {
 			LOGPLCHAN(lchan, DRSL, LOGL_ERROR, "Error parsing MultiRate conf IE\n");
 			rsl_tx_error_report(msg->trx, RSL_ERR_IE_CONTENT, &dch->chan_nr, NULL, msg);
 			return rsl_tx_chan_act_acknack(lchan, RSL_ERR_IE_CONTENT);
 		}
-		memcpy(lchan->mr_bts_lv, TLVP_VAL(&tp, RSL_IE_MR_CONFIG) - 1,
-		       TLVP_LEN(&tp, RSL_IE_MR_CONFIG) + 1);
-		amr_parse_mr_conf(&lchan->tch.amr_mr, TLVP_VAL(&tp, RSL_IE_MR_CONFIG),
-				  TLVP_LEN(&tp, RSL_IE_MR_CONFIG));
-		amr_log_mr_conf(DRTP, LOGL_DEBUG, gsm_lchan_name(lchan),
-				&lchan->tch.amr_mr);
+
+		amr_log_mr_conf(DRTP, LOGL_DEBUG, gsm_lchan_name(lchan), &lchan->tch.amr_mr);
 		lchan->tch.last_cmr = AMR_CMR_NONE;
 	}
 	/* 9.3.53 MultiRate Control */
@@ -1556,6 +1555,7 @@
 	struct gsm_lchan *lchan = msg->lchan;
 	struct rsl_ie_chan_mode *cm;
 	struct tlv_parsed tp;
+	int rc;
 
 	rsl_tlv_parse(&tp, msgb_l3(msg), msgb_l3len(msg));
 
@@ -1588,17 +1588,16 @@
 
 	/* 9.3.52 MultiRate Configuration */
 	if (TLVP_PRESENT(&tp, RSL_IE_MR_CONFIG)) {
-		if (TLVP_LEN(&tp, RSL_IE_MR_CONFIG) > sizeof(lchan->mr_bts_lv) - 1) {
+		rc = amr_parse_mr_conf(&lchan->tch.amr_mr,
+				       TLVP_VAL(&tp, RSL_IE_MR_CONFIG),
+				       TLVP_LEN(&tp, RSL_IE_MR_CONFIG));
+		if (rc < 0) {
 			LOGPLCHAN(lchan, DRSL, LOGL_ERROR, "Error parsing MultiRate conf IE\n");
 			rsl_tx_error_report(msg->trx, RSL_ERR_IE_CONTENT, &dch->chan_nr, NULL, msg);
 			return rsl_tx_mode_modif_nack(lchan, RSL_ERR_IE_CONTENT);;
 		}
-		memcpy(lchan->mr_bts_lv, TLVP_VAL(&tp, RSL_IE_MR_CONFIG) - 1,
-			TLVP_LEN(&tp, RSL_IE_MR_CONFIG) + 1);
-		amr_parse_mr_conf(&lchan->tch.amr_mr, TLVP_VAL(&tp, RSL_IE_MR_CONFIG),
-				  TLVP_LEN(&tp, RSL_IE_MR_CONFIG));
-		amr_log_mr_conf(DRTP, LOGL_DEBUG, gsm_lchan_name(lchan),
-				&lchan->tch.amr_mr);
+
+		amr_log_mr_conf(DRTP, LOGL_DEBUG, gsm_lchan_name(lchan), &lchan->tch.amr_mr);
 		lchan->tch.last_cmr = AMR_CMR_NONE;
 	}
 	/* 9.3.53 MultiRate Control */

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

Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: Ibfd5845ea429945b352dd14421e86562998d65ca
Gerrit-Change-Number: 18680
Gerrit-PatchSet: 2
Gerrit-Owner: fixeria <vyanitskiy at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy at sysmocom.de>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
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/20200605/c8e43259/attachment.htm>