[M] Change in osmocom-bb[master]: mobile: always check return value of tlv_parse()

15 Jan 2024

laforge has submitted this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/35576?usp=email )
Change subject: mobile: always check return value of tlv_parse()
......................................................................
mobile: always check return value of tlv_parse()
Change-Id: Id02fc0b1af6da939cb72f327c7d2ddca484ca063
---
M src/host/layer23/src/mobile/gsm480_ss.c
M src/host/layer23/src/mobile/gsm48_cc.c
M src/host/layer23/src/mobile/gsm48_mm.c
M src/host/layer23/src/mobile/gsm48_rr.c
4 files changed, 143 insertions(+), 64 deletions(-)
Approvals:
  pespin: Looks good to me, but someone else must approve
  Jenkins Builder: Verified
  laforge: Looks good to me, approved

diff --git a/src/host/layer23/src/mobile/gsm480_ss.c b/src/host/layer23/src/mobile/gsm480_ss.c
index 4ed8354..fe601bc 100644
--- a/src/host/layer23/src/mobile/gsm480_ss.c
+++ b/src/host/layer23/src/mobile/gsm480_ss.c
@@ -1062,7 +1062,12 @@
    struct tlv_parsed tp;
    int rc = 0;
-	tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len, 0, 0);
+	if (tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len, 0, 0) < 0) {
+		LOGP(DSS, LOGL_ERROR, "%s(): tlv_parse() failed\n", __func__);
+		gsm480_trans_free(trans);
+		return -EINVAL;
+	}
+
    if (TLVP_PRESENT(&tp, GSM48_IE_FACILITY)) {
    	rc = gsm480_rx_fac_ie(trans, TLVP_VAL(&tp, GSM48_IE_FACILITY),
    		*(TLVP_VAL(&tp, GSM48_IE_FACILITY)-1));
@@ -1095,11 +1100,16 @@
    struct tlv_parsed tp;
    int rc = 0;
+	if (tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len,
+		      GSM48_IE_FACILITY, 0) < 0) {
+		LOGP(DSS, LOGL_ERROR, "%s(): tlv_parse() failed\n", __func__);
+		/* XXX: indicate an error somehow */
+		return -EINVAL;
+	}
+
    /* go register state */
    trans->ss.state = GSM480_SS_ST_ACTIVE;
-	tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len,
-		GSM48_IE_FACILITY, 0);
    if (TLVP_PRESENT(&tp, GSM48_IE_FACILITY)) {
    	rc = gsm480_rx_fac_ie(trans, TLVP_VAL(&tp, GSM48_IE_FACILITY),
    		*(TLVP_VAL(&tp, GSM48_IE_FACILITY)-1));
@@ -1127,10 +1137,15 @@
    struct tlv_parsed tp;
    int rc = 0;
+	if (tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len, 0, 0) < 0) {
+		LOGP(DSS, LOGL_ERROR, "%s(): tlv_parse() failed\n", __func__);
+		/* XXX: indicate an error somehow */
+		return -EINVAL;
+	}
+
    /* go register state */
    trans->ss.state = GSM480_SS_ST_ACTIVE;
-	tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len, 0, 0);
    if (TLVP_PRESENT(&tp, GSM48_IE_FACILITY)) {
    	rc = gsm480_rx_fac_ie(trans, TLVP_VAL(&tp, GSM48_IE_FACILITY),
    		*(TLVP_VAL(&tp, GSM48_IE_FACILITY)-1));
diff --git a/src/host/layer23/src/mobile/gsm48_cc.c b/src/host/layer23/src/mobile/gsm48_cc.c
index 4e26713..c3ec94b 100644
--- a/src/host/layer23/src/mobile/gsm48_cc.c
+++ b/src/host/layer23/src/mobile/gsm48_cc.c
@@ -623,10 +623,14 @@
LOGP(DCC, LOGL_INFO, "received PROGRESS\n");
+	if (tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len,
+		      GSM48_IE_PROGR_IND, 0) < 0) {
+		LOGP(DCC, LOGL_ERROR, "%s(): tlv_parse() failed\n", __func__);
+		return -EINVAL;
+	}
+
    memset(&progress, 0, sizeof(struct gsm_mncc));
    progress.callref = trans->callref;
-	tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len,
-		GSM48_IE_PROGR_IND, 0);
    /* progress */
    if (TLVP_PRESENT(&tp, GSM48_IE_PROGR_IND)) {
    	progress.fields |= MNCC_F_PROGRESS;
@@ -654,13 +658,17 @@
    struct tlv_parsed tp;
    struct gsm_mncc call_proc;
-	LOGP(DCC, LOGL_INFO, "sending CALL PROCEEDING\n");
+	LOGP(DCC, LOGL_INFO, "received CALL PROCEEDING\n");
+
+	if (tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len, 0, 0) < 0) {
+		LOGP(DCC, LOGL_ERROR, "%s(): tlv_parse() failed\n", __func__);
+		return -EINVAL;
+	}
gsm48_stop_cc_timer(trans);
memset(&call_proc, 0, sizeof(struct gsm_mncc));
    call_proc.callref = trans->callref;
-	tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len, 0, 0);
 #if 0
    /* repeat */
    if (TLVP_PRESENT(&tp, GSM48_IE_REPEAT_CIR))
@@ -712,12 +720,16 @@
LOGP(DCC, LOGL_INFO, "received ALERTING\n");
+	if (tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len, 0, 0) < 0) {
+		LOGP(DCC, LOGL_ERROR, "%s(): tlv_parse() failed\n", __func__);
+		return -EINVAL;
+	}
+
    gsm48_stop_cc_timer(trans);
    /* no T301 in MS call control */
memset(&alerting, 0, sizeof(struct gsm_mncc));
    alerting.callref = trans->callref;
-	tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len, 0, 0);
    /* facility */
    if (TLVP_PRESENT(&tp, GSM48_IE_FACILITY)) {
    	alerting.fields |= MNCC_F_FACILITY;
@@ -753,11 +765,15 @@
LOGP(DCC, LOGL_INFO, "received CONNECT\n");
+	if (tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len, 0, 0) < 0) {
+		LOGP(DCC, LOGL_ERROR, "%s(): tlv_parse() failed\n", __func__);
+		return -EINVAL;
+	}
+
    gsm48_stop_cc_timer(trans);
memset(&connect, 0, sizeof(struct gsm_mncc));
    connect.callref = trans->callref;
-	tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len, 0, 0);
    /* facility */
    if (TLVP_PRESENT(&tp, GSM48_IE_FACILITY)) {
    	connect.fields |= MNCC_F_FACILITY;
@@ -823,10 +839,13 @@
LOGP(DCC, LOGL_INFO, "received SETUP\n");
+	if (tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len, 0, 0) < 0) {
+		LOGP(DCC, LOGL_ERROR, "%s(): tlv_parse() failed\n", __func__);
+		return -EINVAL;
+	}
+
    memset(&setup, 0, sizeof(struct gsm_mncc));
    setup.callref = trans->callref;
-	tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len, 0, 0);
-
    /* bearer capability */
    if (TLVP_PRESENT(&tp, GSM48_IE_BEARER_CAP)) {
    	setup.fields |= MNCC_F_BEARER_CAP;
@@ -1076,9 +1095,13 @@
LOGP(DCC, LOGL_INFO, "received START DTMF ACKNOWLEDGE\n");
+	if (tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len, 0, 0) < 0) {
+		LOGP(DCC, LOGL_ERROR, "%s(): tlv_parse() failed\n", __func__);
+		return -EINVAL;
+	}
+
    memset(&dtmf, 0, sizeof(struct gsm_mncc));
    dtmf.callref = trans->callref;
-	tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len, 0, 0);
    /* keypad facility */
    if (TLVP_PRESENT(&tp, GSM48_IE_KPD_FACILITY)) {
    	dtmf.fields |= MNCC_F_KEYPAD;
@@ -1139,9 +1162,13 @@
LOGP(DCC, LOGL_INFO, "received STOP DTMF ACKNOWLEDGE\n");
+	if (tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len, 0, 0) < 0) {
+		LOGP(DCC, LOGL_ERROR, "%s(): tlv_parse() failed\n", __func__);
+		return -EINVAL;
+	}
+
    memset(&dtmf, 0, sizeof(struct gsm_mncc));
    dtmf.callref = trans->callref;
-	tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len, 0, 0);
return mncc_recvmsg(trans->ms, trans, MNCC_STOP_DTMF_RSP, &dtmf);
 }
@@ -1335,15 +1362,18 @@
LOGP(DCC, LOGL_INFO, "received USERINFO\n");
-	memset(&user, 0, sizeof(struct gsm_mncc));
-	user.callref = trans->callref;
    if (payload_len < 1) {
-		LOGP(DCC, LOGL_NOTICE, "Short read of userinfo message "
-			"error.\n");
+		LOGP(DCC, LOGL_NOTICE, "Short read of USERINFO message\n");
    	return -EINVAL;
    }
-	tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len,
-		GSM48_IE_USER_USER, 0);
+	if (tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len,
+		      GSM48_IE_USER_USER, 0) < 0) {
+		LOGP(DCC, LOGL_ERROR, "%s(): tlv_parse() failed\n", __func__);
+		return -EINVAL;
+	}
+
+	memset(&user, 0, sizeof(struct gsm_mncc));
+	user.callref = trans->callref;
    /* user-user */
    gsm48_decode_useruser(&user.useruser,
    		TLVP_VAL(&tp, GSM48_IE_USER_USER)-1);
@@ -1417,17 +1447,20 @@
LOGP(DCC, LOGL_INFO, "received MODIFY REJECT\n");
+	if (payload_len < 1) {
+		LOGP(DCC, LOGL_NOTICE, "Short read of MODIFY REJECT message\n");
+		return -EINVAL;
+	}
+	if (tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len,
+		      GSM48_IE_BEARER_CAP, GSM48_IE_CAUSE) < 0) {
+		LOGP(DCC, LOGL_ERROR, "%s(): tlv_parse() failed\n", __func__);
+		return -EINVAL;
+	}
+
    gsm48_stop_cc_timer(trans);
memset(&modify, 0, sizeof(struct gsm_mncc));
    modify.callref = trans->callref;
-	if (payload_len < 1) {
-		LOGP(DCC, LOGL_NOTICE, "Short read of modify reject message "
-			"error.\n");
-		return -EINVAL;
-	}
-	tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len,
-		GSM48_IE_BEARER_CAP, GSM48_IE_CAUSE);
    /* bearer capability */
    if (TLVP_PRESENT(&tp, GSM48_IE_BEARER_CAP)) {
    	modify.fields |= MNCC_F_BEARER_CAP;
@@ -1675,14 +1708,18 @@
LOGP(DCC, LOGL_INFO, "received DISCONNECT\n");
+	if (tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len,
+		      GSM48_IE_CAUSE, 0) < 0) {
+		LOGP(DCC, LOGL_ERROR, "%s(): tlv_parse() failed\n", __func__);
+		return -EINVAL;
+	}
+
    gsm48_stop_cc_timer(trans);
new_cc_state(trans, GSM_CSTATE_DISCONNECT_IND);
memset(&disc, 0, sizeof(struct gsm_mncc));
    disc.callref = trans->callref;
-	tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len,
-		GSM48_IE_CAUSE, 0);
    /* cause */
    if (TLVP_PRESENT(&tp, GSM48_IE_CAUSE)) {
    	disc.fields |= MNCC_F_CAUSE;
@@ -1724,11 +1761,15 @@
LOGP(DCC, LOGL_INFO, "received RELEASE\n");
+	if (tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len, 0, 0) < 0) {
+		LOGP(DCC, LOGL_ERROR, "%s(): tlv_parse() failed\n", __func__);
+		return -EINVAL;
+	}
+
    gsm48_stop_cc_timer(trans);
memset(&rel, 0, sizeof(struct gsm_mncc));
    rel.callref = trans->callref;
-	tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len, 0, 0);
    /* cause */
    if (TLVP_PRESENT(&tp, GSM48_IE_CAUSE)) {
    	rel.fields |= MNCC_F_CAUSE;
@@ -1793,11 +1834,15 @@
LOGP(DCC, LOGL_INFO, "received RELEASE COMPLETE\n");
+	if (tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len, 0, 0) < 0) {
+		LOGP(DCC, LOGL_ERROR, "%s(): tlv_parse() failed\n", __func__);
+		return -EINVAL;
+	}
+
    gsm48_stop_cc_timer(trans);
memset(&rel, 0, sizeof(struct gsm_mncc));
    rel.callref = trans->callref;
-	tlv_parse(&tp, &gsm48_att_tlvdef, gh->data, payload_len, 0, 0);
    /* cause */
    if (TLVP_PRESENT(&tp, GSM48_IE_CAUSE)) {
    	rel.fields |= MNCC_F_CAUSE;
diff --git a/src/host/layer23/src/mobile/gsm48_mm.c b/src/host/layer23/src/mobile/gsm48_mm.c
index 378cd2d..16a9b07 100644
--- a/src/host/layer23/src/mobile/gsm48_mm.c
+++ b/src/host/layer23/src/mobile/gsm48_mm.c
@@ -2259,11 +2259,13 @@
    struct tlv_parsed tp;
if (payload_len < 0) {
-		LOGP(DMM, LOGL_NOTICE, "Short read of MM INFORMATION message "
-			"error.\n");
+		LOGP(DMM, LOGL_ERROR, "Short read of MM INFORMATION message\n");
    	return -EINVAL;
    }
-	tlv_parse(&tp, &gsm48_mm_att_tlvdef, gh->data, payload_len, 0, 0);
+	if (tlv_parse(&tp, &gsm48_mm_att_tlvdef, gh->data, payload_len, 0, 0) < 0) {
+		LOGP(DMM, LOGL_ERROR, "%s(): tlv_parse() failed\n", __func__);
+		return -EINVAL;
+	}
/* long name */
    if (TLVP_PRESENT(&tp, GSM48_IE_NAME_LONG)) {
@@ -2611,15 +2613,17 @@
    struct tlv_parsed tp;
    struct msgb *nmsg;
-	if (payload_len < sizeof(struct gsm48_loc_area_id)) {
-		short_read:
-		LOGP(DMM, LOGL_NOTICE, "Short read of LOCATION UPDATING ACCEPT "
-			"message error.\n");
+	if (payload_len < 0) {
+short_read:
+		LOGP(DMM, LOGL_ERROR, "Short read of LOCATION UPDATING ACCEPT message\n");
    	return -EINVAL;
    }
-	tlv_parse(&tp, &gsm48_mm_att_tlvdef,
-		gh->data + sizeof(struct gsm48_loc_area_id),
-		payload_len - sizeof(struct gsm48_loc_area_id), 0, 0);
+	if (tlv_parse(&tp, &gsm48_mm_att_tlvdef,
+		      gh->data + sizeof(*lai),
+		      payload_len - sizeof(*lai), 0, 0) < 0) {
+		LOGP(DMM, LOGL_ERROR, "%s(): tlv_parse() failed\n", __func__);
+		return -EINVAL;
+	}
/* update has finished */
    mm->lupd_pending = 0;
diff --git a/src/host/layer23/src/mobile/gsm48_rr.c b/src/host/layer23/src/mobile/gsm48_rr.c
index 334fdaf..46a143c 100644
--- a/src/host/layer23/src/mobile/gsm48_rr.c
+++ b/src/host/layer23/src/mobile/gsm48_rr.c
@@ -3642,12 +3642,13 @@
    struct tlv_parsed tp;
if (payload_len < 0) {
-		LOGP(DRR, LOGL_NOTICE, "Short read of ADDITIONAL ASSIGNMENT "
-			"message.\n");
-		return gsm48_rr_tx_rr_status(ms,
-			GSM48_RR_CAUSE_PROT_ERROR_UNSPC);
+		LOGP(DRR, LOGL_NOTICE, "Short read of ADDITIONAL ASSIGNMENT message\n");
+		return gsm48_rr_tx_rr_status(ms, GSM48_RR_CAUSE_PROT_ERROR_UNSPC);
    }
-	tlv_parse(&tp, &gsm48_rr_att_tlvdef, aa->data, payload_len, 0, 0);
+	if (tlv_parse(&tp, &gsm48_rr_att_tlvdef, aa->data, payload_len, 0, 0) < 0) {
+		LOGP(DRR, LOGL_ERROR, "%s(): tlv_parse() failed\n", __func__);
+		return gsm48_rr_tx_rr_status(ms, GSM48_RR_CAUSE_PROT_ERROR_UNSPC);
+	}
return gsm48_rr_tx_rr_status(ms, GSM48_RR_CAUSE_PROT_ERROR_UNSPC);
 }
@@ -4338,12 +4339,13 @@
    uint8_t *mode;
if (payload_len < 0) {
-		LOGP(DRR, LOGL_NOTICE, "Short read of CHANNEL RELEASE "
-			"message.\n");
-		return gsm48_rr_tx_rr_status(ms,
-			GSM48_RR_CAUSE_PROT_ERROR_UNSPC);
+		LOGP(DRR, LOGL_NOTICE, "Short read of CHANNEL RELEASE message\n");
+		return gsm48_rr_tx_rr_status(ms, GSM48_RR_CAUSE_PROT_ERROR_UNSPC);
    }
-	tlv_parse(&tp, &gsm48_rr_att_tlvdef, cr->data, payload_len, 0, 0);
+	if (tlv_parse(&tp, &gsm48_rr_att_tlvdef, cr->data, payload_len, 0, 0) < 0) {
+		LOGP(DRR, LOGL_ERROR, "%s(): tlv_parse() failed\n", __func__);
+		return gsm48_rr_tx_rr_status(ms, GSM48_RR_CAUSE_PROT_ERROR_UNSPC);
+	}
LOGP(DRR, LOGL_INFO, "channel release request with cause 0x%02x\n",
    	cr->rr_cause);
@@ -4391,10 +4393,13 @@
    struct tlv_parsed tp;
if (payload_len < 0) {
-		LOGP(DRR, LOGL_NOTICE, "Short read of CHANNEL RELEASE message.\n");
+		LOGP(DRR, LOGL_NOTICE, "Short read of CHANNEL RELEASE message\n");
    	return gsm48_rr_tx_rr_status(ms, GSM48_RR_CAUSE_PROT_ERROR_UNSPC);
    }
-	tlv_parse(&tp, &gsm48_rr_att_tlvdef, cr->data, payload_len, 0, 0);
+	if (tlv_parse(&tp, &gsm48_rr_att_tlvdef, cr->data, payload_len, 0, 0) < 0) {
+		LOGP(DRR, LOGL_ERROR, "%s(): tlv_parse() failed\n", __func__);
+		return gsm48_rr_tx_rr_status(ms, GSM48_RR_CAUSE_PROT_ERROR_UNSPC);
+	}
LOGP(DRR, LOGL_INFO, "CHANNEL RELESE via UI frame with cause 0x%02x\n", cr->rr_cause);
@@ -4715,12 +4720,13 @@
    cdb->ind_tx_power = rr->cd_now.ind_tx_power;
if (payload_len < 0) {
-		LOGP(DRR, LOGL_NOTICE, "Short read of ASSIGNMENT COMMAND "
-			"message.\n");
-		return gsm48_rr_tx_rr_status(ms,
-			GSM48_RR_CAUSE_PROT_ERROR_UNSPC);
+		LOGP(DRR, LOGL_NOTICE, "Short read of ASSIGNMENT COMMAND message\n");
+		return gsm48_rr_tx_rr_status(ms, GSM48_RR_CAUSE_PROT_ERROR_UNSPC);
    }
-	tlv_parse(&tp, &gsm48_rr_att_tlvdef, ac->data, payload_len, 0, 0);
+	if (tlv_parse(&tp, &gsm48_rr_att_tlvdef, ac->data, payload_len, 0, 0) < 0) {
+		LOGP(DRR, LOGL_ERROR, "%s(): tlv_parse() failed\n", __func__);
+		return gsm48_rr_tx_rr_status(ms, GSM48_RR_CAUSE_PROT_ERROR_UNSPC);
+	}
/* decode channel description (before time) */
    if (TLVP_PRESENT(&tp, GSM48_IE_CH_DESC_1_BEFORE)) {
@@ -5102,10 +5108,12 @@
    cdb->ind_tx_power = rr->cd_now.ind_tx_power;
if (payload_len < 0) {
-		LOGP(DRR, LOGL_NOTICE, "Short read of HANDOVER COMMAND "
-			"message.\n");
-		return gsm48_rr_tx_rr_status(ms,
-			GSM48_RR_CAUSE_PROT_ERROR_UNSPC);
+		LOGP(DRR, LOGL_NOTICE, "Short read of HANDOVER COMMAND message\n");
+		return gsm48_rr_tx_rr_status(ms, GSM48_RR_CAUSE_PROT_ERROR_UNSPC);
+	}
+	if (tlv_parse(&tp, &gsm48_rr_att_tlvdef, ho->data, payload_len, 0, 0) < 0) {
+		LOGP(DRR, LOGL_ERROR, "%s(): tlv_parse() failed\n", __func__);
+		return gsm48_rr_tx_rr_status(ms, GSM48_RR_CAUSE_PROT_ERROR_UNSPC);
    }
/* cell description */
@@ -5115,8 +5123,6 @@
    rr->chan_req_val = ho->ho_ref;
    rr->chan_req_mask = 0x00;
-	tlv_parse(&tp, &gsm48_rr_att_tlvdef, ho->data, payload_len, 0, 0);
-
    /* sync ind */
    if (TLVP_PRESENT(&tp, GSM48_IE_SYNC_IND)) {
    	gsm48_decode_sync_ind(rr, (struct gsm48_sync_ind *)
-- 
To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/35576?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Change-Id: Id02fc0b1af6da939cb72f327c7d2ddca484ca063
Gerrit-Change-Number: 35576
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria vyanitskiy@sysmocom.de
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge laforge@osmocom.org
Gerrit-Reviewer: pespin pespin@sysmocom.de
Gerrit-MessageType: merged



    

2025

2024

2023

2022

[M] Change in osmocom-bb[master]: mobile: always check return value of tlv_parse()