Change in libosmocore[master]: bssgp: Use TLVP_PRES_LEN instead of TLVP_PRESENT

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

laforge gerrit-no-reply at lists.osmocom.org
Fri Dec 4 18:20:15 UTC 2020


laforge has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/21493 )

Change subject: bssgp: Use TLVP_PRES_LEN instead of TLVP_PRESENT
......................................................................

bssgp: Use TLVP_PRES_LEN instead of TLVP_PRESENT

With TLVP_PRESENT we only check if a given TLV/IE is present,
but don't verify that it's length matches our expectation.  This can
lead to out-of-bounds reads, so let's always use TLVP_PRES_LEN.

Change-Id: I56e8b31ce51602d2681e3db501c48f84bfe7e438
---
M src/gb/gprs_bssgp.c
M src/gb/gprs_bssgp_bss.c
2 files changed, 32 insertions(+), 33 deletions(-)

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



diff --git a/src/gb/gprs_bssgp.c b/src/gb/gprs_bssgp.c
index ebbfab1..7fb3a30 100644
--- a/src/gb/gprs_bssgp.c
+++ b/src/gb/gprs_bssgp.c
@@ -351,7 +351,7 @@
 	/* When we receive a BVC-RESET PDU (at least of a PTP BVCI), the BSS
 	 * informs us about its RAC + Cell ID, so we can create a mapping */
 	if (bvci != 0 && bvci != 1) {
-		if (!TLVP_PRESENT(tp, BSSGP_IE_CELL_ID)) {
+		if (!TLVP_PRES_LEN(tp, BSSGP_IE_CELL_ID, 8)) {
 			LOGP(DBSSGP, LOGL_ERROR, "BSSGP BVCI=%u Rx RESET "
 				"missing mandatory IE\n", bvci);
 			return -EINVAL;
@@ -467,7 +467,7 @@
 	DEBUGP(DBSSGP, "BSSGP TLLI=0x%08x Rx UPLINK-UNITDATA\n", msgb_tlli(msg));
 
 	/* Cell ID and LLC_PDU are the only mandatory IE */
-	if (!TLVP_PRESENT(tp, BSSGP_IE_CELL_ID) ||
+	if (!TLVP_PRES_LEN(tp, BSSGP_IE_CELL_ID, 8) ||
 	    !TLVP_PRESENT(tp, BSSGP_IE_LLC_PDU)) {
 		LOGP(DBSSGP, LOGL_ERROR, "BSSGP TLLI=0x%08x Rx UL-UD "
 			"missing mandatory IE\n", msgb_tlli(msg));
@@ -497,8 +497,8 @@
 	uint16_t ns_bvci = msgb_bvci(msg), nsei = msgb_nsei(msg);
 	int rc;
 
-	if (!TLVP_PRESENT(tp, BSSGP_IE_TLLI) ||
-	    !TLVP_PRESENT(tp, BSSGP_IE_ROUTEING_AREA)) {
+	if (!TLVP_PRES_LEN(tp, BSSGP_IE_TLLI, 4) ||
+	    !TLVP_PRES_LEN(tp, BSSGP_IE_ROUTEING_AREA, 6)) {
 		LOGP(DBSSGP, LOGL_ERROR, "BSSGP BVCI=%u Rx SUSPEND "
 			"missing mandatory IE\n", ns_bvci);
 		return bssgp_tx_status(BSSGP_CAUSE_MISSING_MAND_IE, NULL, msg);
@@ -538,9 +538,9 @@
 	uint16_t ns_bvci = msgb_bvci(msg), nsei = msgb_nsei(msg);
 	int rc;
 
-	if (!TLVP_PRESENT(tp, BSSGP_IE_TLLI) ||
-	    !TLVP_PRESENT(tp, BSSGP_IE_ROUTEING_AREA) ||
-	    !TLVP_PRESENT(tp, BSSGP_IE_SUSPEND_REF_NR)) {
+	if (!TLVP_PRES_LEN(tp, BSSGP_IE_TLLI, 4 ) ||
+	    !TLVP_PRES_LEN(tp, BSSGP_IE_ROUTEING_AREA, 6) ||
+	    !TLVP_PRES_LEN(tp, BSSGP_IE_SUSPEND_REF_NR, 1)) {
 		LOGP(DBSSGP, LOGL_ERROR, "BSSGP BVCI=%u Rx RESUME "
 			"missing mandatory IE\n", ns_bvci);
 		return bssgp_tx_status(BSSGP_CAUSE_MISSING_MAND_IE, NULL, msg);
@@ -580,16 +580,16 @@
 	uint32_t tlli = 0;
 	uint16_t nsei = msgb_nsei(msg);
 
-	if (!TLVP_PRESENT(tp, BSSGP_IE_TLLI) ||
-	    !TLVP_PRESENT(tp, BSSGP_IE_LLC_FRAMES_DISCARDED) ||
-	    !TLVP_PRESENT(tp, BSSGP_IE_BVCI) ||
-	    !TLVP_PRESENT(tp, BSSGP_IE_NUM_OCT_AFF)) {
+	if (!TLVP_PRES_LEN(tp, BSSGP_IE_TLLI, 4) ||
+	    !TLVP_PRES_LEN(tp, BSSGP_IE_LLC_FRAMES_DISCARDED, 1) ||
+	    !TLVP_PRES_LEN(tp, BSSGP_IE_BVCI, 2) ||
+	    !TLVP_PRES_LEN(tp, BSSGP_IE_NUM_OCT_AFF, 3)) {
 		LOGP(DBSSGP, LOGL_ERROR, "BSSGP BVCI=%u Rx LLC DISCARDED "
 			"missing mandatory IE\n", ctx->bvci);
+		return bssgp_tx_status(BSSGP_CAUSE_MISSING_MAND_IE, NULL, msg);
 	}
 
-	if (TLVP_PRESENT(tp, BSSGP_IE_TLLI))
-		tlli = tlvp_val32be(tp, BSSGP_IE_TLLI);
+	tlli = tlvp_val32be(tp, BSSGP_IE_TLLI);
 
 	DEBUGP(DBSSGP, "BSSGP BVCI=%u TLLI=%08x Rx LLC DISCARDED\n",
 		ctx->bvci, tlli);
@@ -615,7 +615,7 @@
 	struct osmo_bssgp_prim nmp;
 	enum gprs_bssgp_cause cause;
 
-	if (!TLVP_PRESENT(tp, BSSGP_IE_CAUSE)) {
+	if (!TLVP_PRES_LEN(tp, BSSGP_IE_CAUSE, 1)) {
 		LOGP(DBSSGP, LOGL_ERROR, "BSSGP BVCI=%u Rx STATUS "
 			"missing mandatory IE\n", bvci);
 		cause = BSSGP_CAUSE_PROTO_ERR_UNSPEC;
@@ -627,7 +627,7 @@
 		bvci, bssgp_cause_str(cause));
 
 	if (cause == BSSGP_CAUSE_BVCI_BLOCKED || cause == BSSGP_CAUSE_UNKNOWN_BVCI) {
-		if (!TLVP_PRESENT(tp, BSSGP_IE_BVCI))
+		if (!TLVP_PRES_LEN(tp, BSSGP_IE_BVCI, 2))
 			LOGP(DBSSGP, LOGL_ERROR,
 				"BSSGP BVCI=%u Rx STATUS cause=%s "
 				"missing conditional BVCI IE\n",
@@ -882,11 +882,11 @@
 	DEBUGP(DBSSGP, "BSSGP BVCI=%u Rx Flow Control BVC\n",
 		bctx->bvci);
 
-	if (!TLVP_PRESENT(tp, BSSGP_IE_TAG) ||
-	    !TLVP_PRESENT(tp, BSSGP_IE_BVC_BUCKET_SIZE) ||
-	    !TLVP_PRESENT(tp, BSSGP_IE_BUCKET_LEAK_RATE) ||
-	    !TLVP_PRESENT(tp, BSSGP_IE_BMAX_DEFAULT_MS) ||
-	    !TLVP_PRESENT(tp, BSSGP_IE_R_DEFAULT_MS)) {
+	if (!TLVP_PRES_LEN(tp, BSSGP_IE_TAG, 1) ||
+	    !TLVP_PRES_LEN(tp, BSSGP_IE_BVC_BUCKET_SIZE, 2) ||
+	    !TLVP_PRES_LEN(tp, BSSGP_IE_BUCKET_LEAK_RATE, 2) ||
+	    !TLVP_PRES_LEN(tp, BSSGP_IE_BMAX_DEFAULT_MS, 2) ||
+	    !TLVP_PRES_LEN(tp, BSSGP_IE_R_DEFAULT_MS,2)) {
 		LOGP(DBSSGP, LOGL_ERROR, "BSSGP BVCI=%u Rx FC BVC "
 			"missing mandatory IE\n", bctx->bvci);
 		return bssgp_tx_status(BSSGP_CAUSE_MISSING_MAND_IE, NULL, msg);
@@ -1040,8 +1040,8 @@
 		break;
 	case BSSGP_PDUT_BVC_BLOCK:
 		/* BSS tells us that BVC shall be blocked */
-		if (!TLVP_PRESENT(tp, BSSGP_IE_BVCI) ||
-		    !TLVP_PRESENT(tp, BSSGP_IE_CAUSE)) {
+		if (!TLVP_PRES_LEN(tp, BSSGP_IE_BVCI, 2) ||
+		    !TLVP_PRES_LEN(tp, BSSGP_IE_CAUSE, 1)) {
 			LOGP(DBSSGP, LOGL_ERROR, "BSSGP Rx BVC-BLOCK "
 				"missing mandatory IE\n");
 			goto err_mand_ie;
@@ -1050,7 +1050,7 @@
 		break;
 	case BSSGP_PDUT_BVC_UNBLOCK:
 		/* BSS tells us that BVC shall be unblocked */
-		if (!TLVP_PRESENT(tp, BSSGP_IE_BVCI)) {
+		if (!TLVP_PRES_LEN(tp, BSSGP_IE_BVCI, 2)) {
 			LOGP(DBSSGP, LOGL_ERROR, "BSSGP Rx BVC-UNBLOCK "
 				"missing mandatory IE\n");
 			goto err_mand_ie;
@@ -1062,8 +1062,8 @@
 		break;
 	case BSSGP_PDUT_BVC_RESET:
 		/* BSS tells us that BVC init is required */
-		if (!TLVP_PRESENT(tp, BSSGP_IE_BVCI) ||
-		    !TLVP_PRESENT(tp, BSSGP_IE_CAUSE)) {
+		if (!TLVP_PRES_LEN(tp, BSSGP_IE_BVCI, 2) ||
+		    !TLVP_PRES_LEN(tp, BSSGP_IE_CAUSE, 1)) {
 			LOGP(DBSSGP, LOGL_ERROR, "BSSGP Rx BVC-RESET "
 				"missing mandatory IE\n");
 			goto err_mand_ie;
@@ -1135,7 +1135,7 @@
 		return rc;
 	}
 
-	if (bvci == BVCI_SIGNALLING && TLVP_PRESENT(&tp, BSSGP_IE_BVCI))
+	if (bvci == BVCI_SIGNALLING && TLVP_PRES_LEN(&tp, BSSGP_IE_BVCI, 2))
 		bvci = tlvp_val16be(&tp, BSSGP_IE_BVCI);
 
 	/* look-up or create the BTS context for this BVC */
diff --git a/src/gb/gprs_bssgp_bss.c b/src/gb/gprs_bssgp_bss.c
index 59b06f0..462666a 100644
--- a/src/gb/gprs_bssgp_bss.c
+++ b/src/gb/gprs_bssgp_bss.c
@@ -511,24 +511,24 @@
 			   TLVP_LEN(&tp, BSSGP_IE_IMSI));
 
 	/* DRX Parameters */
-	if (!TLVP_PRESENT(&tp, BSSGP_IE_DRX_PARAMS))
+	if (!TLVP_PRES_LEN(&tp, BSSGP_IE_DRX_PARAMS, 2))
 		goto err_mand_ie;
 	pinfo->drx_params = tlvp_val16be(&tp, BSSGP_IE_DRX_PARAMS);
 
 	/* Scope */
-	if (TLVP_PRESENT(&tp, BSSGP_IE_BSS_AREA_ID)) {
+	if (TLVP_PRES_LEN(&tp, BSSGP_IE_BSS_AREA_ID, 1)) {
 		pinfo->scope = BSSGP_PAGING_BSS_AREA;
-	} else if (TLVP_PRESENT(&tp, BSSGP_IE_LOCATION_AREA)) {
+	} else if (TLVP_PRES_LEN(&tp, BSSGP_IE_LOCATION_AREA, 5)) {
 		pinfo->scope = BSSGP_PAGING_LOCATION_AREA;
 		memcpy(ra, TLVP_VAL(&tp, BSSGP_IE_LOCATION_AREA),
 			TLVP_LEN(&tp, BSSGP_IE_LOCATION_AREA));
 		gsm48_parse_ra(&pinfo->raid, ra);
-	} else if (TLVP_PRESENT(&tp, BSSGP_IE_ROUTEING_AREA)) {
+	} else if (TLVP_PRES_LEN(&tp, BSSGP_IE_ROUTEING_AREA, 6)) {
 		pinfo->scope = BSSGP_PAGING_ROUTEING_AREA;
 		memcpy(ra, TLVP_VAL(&tp, BSSGP_IE_ROUTEING_AREA),
 			TLVP_LEN(&tp, BSSGP_IE_ROUTEING_AREA));
 		gsm48_parse_ra(&pinfo->raid, ra);
-	} else if (TLVP_PRESENT(&tp, BSSGP_IE_BVCI)) {
+	} else if (TLVP_PRES_LEN(&tp, BSSGP_IE_BVCI, 2)) {
 		pinfo->scope = BSSGP_PAGING_BVCI;
 		pinfo->bvci = tlvp_val16be(&tp, BSSGP_IE_BVCI);
 	} else
@@ -546,8 +546,7 @@
 	}
 
 	/* Optional (P-)TMSI */
-	if (TLVP_PRESENT(&tp, BSSGP_IE_TMSI) &&
-	    TLVP_LEN(&tp, BSSGP_IE_TMSI) >= 4) {
+	if (TLVP_PRES_LEN(&tp, BSSGP_IE_TMSI, 4)) {
 		if (!pinfo->ptmsi)
 			pinfo->ptmsi = talloc_zero_size(pinfo, sizeof(uint32_t));
 		*(pinfo->ptmsi) = osmo_load32be(TLVP_VAL(&tp, BSSGP_IE_TMSI));

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

Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I56e8b31ce51602d2681e3db501c48f84bfe7e438
Gerrit-Change-Number: 21493
Gerrit-PatchSet: 6
Gerrit-Owner: laforge <laforge at osmocom.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann at sysmocom.de>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
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/20201204/ec9aa913/attachment.htm>


More information about the gerrit-log mailing list