paging: pass struct osmo_mobile_identity, not encoded IE bytes

In get_paging_mi(), before this, an encoded buffer of Mobile Identity bytes is
returned. Code paths following this repeatedly decode the Mobile Identity
bytes, e.g. for logging. Also, in get_paging_mi(), since the TMSI is read in
from a different encoding than a typical Mobile Identity IE, the TMSI was
manually encoded into a typical Mobile Identity IE. This is essentially a code
dup of osmo_mobile_identity_encode(). Stop this madness.

Instead, in get_paging_mi(), return a decoded struct osmo_mobile_identity. Code
paths after this use the struct osmo_mobile_identity directly without repeated
decoding.

At the point of finally needing an encoded Mobile Identity IE (in
Encoding::write_paging_request()), do a proper osmo_mobile_identity_encode().

Since this may return errors, add an rc check for the caller of
write_paging_request(), gprs_rlcmac_paging_request().

A side effect is stricter validation of the Mobile Identity passing through the
Paging code path. Before, invalid MI might have passed through unnoticed.

Change-Id: Iad845acb0096b75dc453105c9c16b2252879b4ca

10 files changed, 58 insertions, 40 deletions

diff --git a/src/bts.cpp b/src/bts.cpp
index c415dd43..76ca1b02 100644
--- a/src/bts.cpp
+++ b/src/bts.cpp
@@ -355,7 +355,7 @@ void BTS::set_current_block_frame_number(int fn, unsigned max_delay)
 	m_pollController.expireTimedout(fn, max_delay);
 }
 
-int BTS::add_paging(uint8_t chan_needed, const uint8_t *mi, uint8_t mi_len)
+int BTS::add_paging(uint8_t chan_needed, const struct osmo_mobile_identity *mi)
 {
 	uint8_t l, trx, ts, any_tbf = 0;
 	struct gprs_rlcmac_tbf *tbf;
@@ -370,10 +370,8 @@ int BTS::add_paging(uint8_t chan_needed, const uint8_t *mi, uint8_t mi_len)
 	};
 
 	if (log_check_level(DRLCMAC, LOGL_INFO)) {
-		struct osmo_mobile_identity omi = {};
 		char str[64];
-		osmo_mobile_identity_decode(&omi, mi, mi_len, true);
-		osmo_mobile_identity_to_str_buf(str, sizeof(str), &omi);
+		osmo_mobile_identity_to_str_buf(str, sizeof(str), mi);
 		LOGP(DRLCMAC, LOGL_INFO, "Add RR paging: chan-needed=%d MI=%s\n", chan_needed, str);
 	}
 
@@ -419,7 +417,7 @@ int BTS::add_paging(uint8_t chan_needed, const uint8_t *mi, uint8_t mi_len)
 		for (ts = 0; ts < 8; ts++) {
 			if ((slot_mask[trx] & (1 << ts))) {
 				/* schedule */
-				if (!m_bts.trx[trx].pdch[ts].add_paging(chan_needed, mi, mi_len))
+				if (!m_bts.trx[trx].pdch[ts].add_paging(chan_needed, mi))
 					return -ENOMEM;
 
 				LOGP(DRLCMAC, LOGL_INFO, "Paging on PACCH of TRX=%d TS=%d\n", trx, ts);
diff --git a/src/bts.h b/src/bts.h
index edccc280..5a1b162c 100644
--- a/src/bts.h
+++ b/src/bts.h
@@ -309,7 +309,7 @@ public:
 	int current_frame_number() const;
 
 	/** add paging to paging queue(s) */
-	int add_paging(uint8_t chan_needed, const uint8_t *mi, uint8_t mi_len);
+	int add_paging(uint8_t chan_needed, const struct osmo_mobile_identity *mi);
 
 	gprs_rlcmac_dl_tbf *dl_tbf_by_poll_fn(uint32_t fn, uint8_t trx, uint8_t ts);
 	gprs_rlcmac_ul_tbf *ul_tbf_by_poll_fn(uint32_t fn, uint8_t trx, uint8_t ts);
diff --git a/src/encoding.cpp b/src/encoding.cpp
index 9dfd7c93..881dc61a 100644
--- a/src/encoding.cpp
+++ b/src/encoding.cpp
@@ -30,6 +30,7 @@
 extern "C" {
 #include <osmocom/gprs/protocol/gsm_04_60.h>
 #include <osmocom/gsm/protocol/gsm_04_08.h>
+#include <osmocom/gsm/gsm48.h>
 }
 
 #include <stdbool.h>
@@ -720,8 +721,10 @@ void Encoding::write_packet_downlink_assignment(RlcMacDownlink_t * block,
 }
 
 /* Generate paging request. See 44.018, sections 10 and 9.1.22 */
-int Encoding::write_paging_request(bitvec * dest, const uint8_t *mi, uint8_t mi_len)
+int Encoding::write_paging_request(bitvec * dest, const struct osmo_mobile_identity *mi)
 {
+	uint8_t mi_buf[GSM48_MID_MAX_SIZE];
+	int mi_len;
 	unsigned wp = 0;
 	int plen;
 
@@ -732,8 +735,11 @@ int Encoding::write_paging_request(bitvec * dest, const uint8_t *mi, uint8_t mi_
 	bitvec_write_field(dest, &wp,0x0,4);  // Page Mode
 	bitvec_write_field(dest, &wp,0x0,4);  // Channel Needed
 
+	mi_len = osmo_mobile_identity_encode_buf(mi_buf, sizeof(mi_buf), mi, true);
+	if (mi_len <= 0)
+		return mi_len;
 	bitvec_write_field(dest, &wp, mi_len, 8);  // Mobile Identity length
-	bitvec_set_bytes(dest, mi, mi_len);        // Mobile Identity
+	bitvec_set_bytes(dest, mi_buf, mi_len);    // Mobile Identity
 	wp += mi_len * 8;
 
 	OSMO_ASSERT(wp % 8 == 0);
diff --git a/src/encoding.h b/src/encoding.h
index 5bdd4ef9..2f6b273f 100644
--- a/src/encoding.h
+++ b/src/encoding.h
@@ -78,7 +78,7 @@ public:
 			bitvec * dest, struct gprs_rlcmac_ul_tbf *tbf, bool is_final,
 			uint8_t rrbp);
 
-	static int write_paging_request(bitvec * dest, const uint8_t *mi, uint8_t mi_len);
+	static int write_paging_request(bitvec * dest, const struct osmo_mobile_identity *mi);
 
 	static unsigned write_repeated_page_info(bitvec * dest, unsigned& wp, uint8_t len,
 			uint8_t *identity, uint8_t chan_needed);
diff --git a/src/gprs_bssgp_pcu.cpp b/src/gprs_bssgp_pcu.cpp
index a46852a3..14d1a9d2 100644
--- a/src/gprs_bssgp_pcu.cpp
+++ b/src/gprs_bssgp_pcu.cpp
@@ -171,11 +171,8 @@ static int gprs_bssgp_pcu_rx_dl_ud(struct msgb *msg, struct tlv_parsed *tp)
 }
 
 /* Returns 0 on success, suggested BSSGP cause otherwise */
-static unsigned int get_paging_mi(const uint8_t **mi, uint8_t *mi_len,
-				  const struct tlv_parsed *tp)
+static unsigned int get_paging_mi(struct osmo_mobile_identity *mi, const struct tlv_parsed *tp)
 {
-	static uint8_t tmsi_buf[GSM48_TMSI_LEN];
-
 	/* Use TMSI (if present) or IMSI */
 	if (TLVP_PRESENT(tp, BSSGP_IE_TMSI)) {
 		/* Be safe against an evil SGSN - check the length */
@@ -185,14 +182,17 @@ static unsigned int get_paging_mi(const uint8_t **mi, uint8_t *mi_len,
 		}
 
 		/* NOTE: TMSI (unlike IMSI) IE comes without MI type header */
-		memcpy(tmsi_buf + 1, TLVP_VAL(tp, BSSGP_IE_TMSI), GSM23003_TMSI_NUM_BYTES);
-		tmsi_buf[0] = 0xf0 | GSM_MI_TYPE_TMSI;
-
-		*mi_len = GSM48_TMSI_LEN;
-		*mi = tmsi_buf;
+		*mi = (struct osmo_mobile_identity){
+			.type = GSM_MI_TYPE_TMSI,
+		};
+		mi->tmsi = osmo_load32be(TLVP_VAL(tp, BSSGP_IE_TMSI));
 	} else if (TLVP_PRESENT(tp, BSSGP_IE_IMSI)) {
-		*mi_len = TLVP_LEN(tp, BSSGP_IE_IMSI);
-		*mi = TLVP_VAL(tp, BSSGP_IE_IMSI);
+		int rc = osmo_mobile_identity_decode(mi, TLVP_VAL(tp, BSSGP_IE_IMSI), TLVP_LEN(tp, BSSGP_IE_IMSI),
+						     true);
+		if (rc < 0 || mi->type != GSM_MI_TYPE_IMSI) {
+			LOGP(DBSSGP, LOGL_ERROR, "Invalid IMSI Mobile Identity\n");
+			return BSSGP_CAUSE_COND_IE_ERR;
+		}
 	} else {
 		LOGP(DBSSGP, LOGL_ERROR, "Neither TMSI IE nor IMSI IE is present\n");
 		return BSSGP_CAUSE_MISSING_COND_IE;
@@ -203,23 +203,21 @@ static unsigned int get_paging_mi(const uint8_t **mi, uint8_t *mi_len,
 
 static int gprs_bssgp_pcu_rx_paging_cs(struct msgb *msg, struct tlv_parsed *tp)
 {
-	const uint8_t *mi;
-	uint8_t mi_len;
+	struct osmo_mobile_identity mi;
 	int rc;
 	uint8_t *chan_needed = (uint8_t *)TLVP_VAL(tp, BSSGP_IE_CHAN_NEEDED);
 
-	if ((rc = get_paging_mi(&mi, &mi_len, tp)) > 0)
+	if ((rc = get_paging_mi(&mi, tp)) > 0)
 		return bssgp_tx_status((enum gprs_bssgp_cause) rc, NULL, msg);
 
-	return BTS::main_bts()->add_paging(chan_needed ? *chan_needed : 0, mi, mi_len);
+	return BTS::main_bts()->add_paging(chan_needed ? *chan_needed : 0, &mi);
 }
 
 static int gprs_bssgp_pcu_rx_paging_ps(struct msgb *msg, struct tlv_parsed *tp)
 {
 	struct osmo_mobile_identity mi_imsi;
+	struct osmo_mobile_identity paging_mi;
 	uint16_t pgroup;
-	const uint8_t *mi;
-	uint8_t mi_len;
 	int rc;
 
 	if (!TLVP_PRESENT(tp, BSSGP_IE_IMSI)) {
@@ -238,10 +236,10 @@ static int gprs_bssgp_pcu_rx_paging_ps(struct msgb *msg, struct tlv_parsed *tp)
 		return bssgp_tx_status(BSSGP_CAUSE_INV_MAND_INF, NULL, msg);
 	}
 
-	if ((rc = get_paging_mi(&mi, &mi_len, tp)) > 0)
+	if ((rc = get_paging_mi(&paging_mi, tp)) > 0)
 		return bssgp_tx_status((enum gprs_bssgp_cause) rc, NULL, msg);
 
-	return gprs_rlcmac_paging_request(mi, mi_len, pgroup);
+	return gprs_rlcmac_paging_request(&paging_mi, pgroup);
 }
 
 /* Receive a BSSGP PDU from a BSS on a PTP BVCI */
diff --git a/src/gprs_rlcmac.cpp b/src/gprs_rlcmac.cpp
index 879ea85d..4fb75f77 100644
--- a/src/gprs_rlcmac.cpp
+++ b/src/gprs_rlcmac.cpp
@@ -32,18 +32,20 @@ extern "C" {
 
 extern void *tall_pcu_ctx;
 
-int gprs_rlcmac_paging_request(const uint8_t *mi, uint8_t mi_len, uint16_t pgroup)
+int gprs_rlcmac_paging_request(const struct osmo_mobile_identity *mi, uint16_t pgroup)
 {
 	if (log_check_level(DRLCMAC, LOGL_NOTICE)) {
-		struct osmo_mobile_identity omi = {};
 		char str[64];
-		osmo_mobile_identity_decode(&omi, mi, mi_len, true);
-		osmo_mobile_identity_to_str_buf(str, sizeof(str), &omi);
+		osmo_mobile_identity_to_str_buf(str, sizeof(str), mi);
 		LOGP(DRLCMAC, LOGL_NOTICE, "TX: [PCU -> BTS] Paging Request (CCCH) MI=%s\n", str);
 	}
 	bitvec *paging_request = bitvec_alloc(22, tall_pcu_ctx);
 	bitvec_unhex(paging_request, DUMMY_VEC);
-	int plen = Encoding::write_paging_request(paging_request, mi, mi_len);
+	int plen = Encoding::write_paging_request(paging_request, mi);
+	if (plen <= 0) {
+		LOGP(DRLCMAC, LOGL_ERROR, "TX: [PCU -> BTS] Failed to encode Paging Request\n");
+		return -1;
+	}
 	pcu_l1if_tx_pch(paging_request, plen, pgroup);
 	bitvec_free(paging_request);
 
diff --git a/src/gprs_rlcmac.h b/src/gprs_rlcmac.h
index 789b8dbd..6587c400 100644
--- a/src/gprs_rlcmac.h
+++ b/src/gprs_rlcmac.h
@@ -93,7 +93,7 @@ enum gprs_rlcmac_block_type {
 
 int gprs_rlcmac_tx_ul_ud(gprs_rlcmac_tbf *tbf);
 
-int gprs_rlcmac_paging_request(const uint8_t *mi, uint8_t mi_len, uint16_t pgroup);
+int gprs_rlcmac_paging_request(const struct osmo_mobile_identity *mi, uint16_t pgroup);
 
 struct msgb *gprs_rlcmac_app_info_msg(const struct gsm_pcu_if_app_info_req *req);
 
diff --git a/src/pcu_l1_if.cpp b/src/pcu_l1_if.cpp
index 7fa82fb5..07124709 100644
--- a/src/pcu_l1_if.cpp
+++ b/src/pcu_l1_if.cpp
@@ -677,6 +677,9 @@ static int pcu_rx_time_ind(struct gsm_pcu_if_time_ind *time_ind)
 
 static int pcu_rx_pag_req(struct gsm_pcu_if_pag_req *pag_req)
 {
+	struct osmo_mobile_identity mi;
+	int rc;
+
 	LOGP(DL1IF, LOGL_DEBUG, "Paging request received: chan_needed=%d "
 		"length=%d\n", pag_req->chan_needed, pag_req->identity_lv[0]);
 
@@ -687,8 +690,13 @@ static int pcu_rx_pag_req(struct gsm_pcu_if_pag_req *pag_req)
 		return -EINVAL;
 	}
 
-	return BTS::main_bts()->add_paging(pag_req->chan_needed, &pag_req->identity_lv[1],
-					   pag_req->identity_lv[0]);
+	rc = osmo_mobile_identity_decode(&mi, &pag_req->identity_lv[1], pag_req->identity_lv[0], true);
+	if (rc < 0) {
+		LOGP(DL1IF, LOGL_ERROR, "Failed to decode Mobile Identity in Paging Request (rc=%d)\n", rc);
+		return -EINVAL;
+	}
+
+	return BTS::main_bts()->add_paging(pag_req->chan_needed, &mi);
 }
 
 static int pcu_rx_susp_req(struct gsm_pcu_if_susp_req *susp_req)
diff --git a/src/pdch.cpp b/src/pdch.cpp
index af847241..fbbeddc5 100644
--- a/src/pdch.cpp
+++ b/src/pdch.cpp
@@ -266,15 +266,21 @@ free_ret:
 	return NULL;
 }
 
-bool gprs_rlcmac_pdch::add_paging(uint8_t chan_needed, const uint8_t *mi, uint8_t mi_len)
+bool gprs_rlcmac_pdch::add_paging(uint8_t chan_needed, const struct osmo_mobile_identity *mi)
 {
+	int rc;
 	struct gprs_rlcmac_paging *pag = talloc_zero(tall_pcu_ctx, struct gprs_rlcmac_paging);
 	if (!pag)
 		return false;
 
 	pag->chan_needed = chan_needed;
-	pag->identity_lv[0] = mi_len;
-	memcpy(&pag->identity_lv[1], mi, mi_len);
+	rc = osmo_mobile_identity_encode_buf(pag->identity_lv + 1, sizeof(pag->identity_lv) - 1, mi, true);
+	if (rc <= 0) {
+		LOGP(DRLCMAC, LOGL_ERROR, "Cannot encode Mobile Identity (rc=%d)\n", rc);
+		talloc_free(pag);
+		return false;
+	}
+	pag->identity_lv[0] = rc;
 
 	llist_add(&pag->list, &paging_list);
 
diff --git a/src/pdch.h b/src/pdch.h
index ec351746..e1e3688f 100644
--- a/src/pdch.h
+++ b/src/pdch.h
@@ -48,7 +48,7 @@ struct gprs_rlcmac_pdch {
 	struct gprs_rlcmac_paging *dequeue_paging();
 	struct msgb *packet_paging_request();
 
-	bool add_paging(uint8_t chan_needed, const uint8_t *mi, uint8_t mi_len);
+	bool add_paging(uint8_t chan_needed, const struct osmo_mobile_identity *mi);
 
 	void free_resources();