<p>laforge <strong>submitted</strong> this change.</p><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/21493">View Change</a></p><div style="white-space:pre-wrap">Approvals:
daniel: Looks good to me, but someone else must approve
pespin: Looks good to me, approved
Jenkins Builder: Verified
</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">bssgp: Use TLVP_PRES_LEN instead of TLVP_PRESENT<br><br>With TLVP_PRESENT we only check if a given TLV/IE is present,<br>but don't verify that it's length matches our expectation. This can<br>lead to out-of-bounds reads, so let's always use TLVP_PRES_LEN.<br><br>Change-Id: I56e8b31ce51602d2681e3db501c48f84bfe7e438<br>---<br>M src/gb/gprs_bssgp.c<br>M src/gb/gprs_bssgp_bss.c<br>2 files changed, 32 insertions(+), 33 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/src/gb/gprs_bssgp.c b/src/gb/gprs_bssgp.c</span><br><span>index ebbfab1..7fb3a30 100644</span><br><span>--- a/src/gb/gprs_bssgp.c</span><br><span>+++ b/src/gb/gprs_bssgp.c</span><br><span>@@ -351,7 +351,7 @@</span><br><span> /* When we receive a BVC-RESET PDU (at least of a PTP BVCI), the BSS</span><br><span> * informs us about its RAC + Cell ID, so we can create a mapping */</span><br><span> if (bvci != 0 && bvci != 1) {</span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(tp, BSSGP_IE_CELL_ID)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(tp, BSSGP_IE_CELL_ID, 8)) {</span><br><span> LOGP(DBSSGP, LOGL_ERROR, "BSSGP BVCI=%u Rx RESET "</span><br><span> "missing mandatory IE\n", bvci);</span><br><span> return -EINVAL;</span><br><span>@@ -467,7 +467,7 @@</span><br><span> DEBUGP(DBSSGP, "BSSGP TLLI=0x%08x Rx UPLINK-UNITDATA\n", msgb_tlli(msg));</span><br><span> </span><br><span> /* Cell ID and LLC_PDU are the only mandatory IE */</span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(tp, BSSGP_IE_CELL_ID) ||</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(tp, BSSGP_IE_CELL_ID, 8) ||</span><br><span> !TLVP_PRESENT(tp, BSSGP_IE_LLC_PDU)) {</span><br><span> LOGP(DBSSGP, LOGL_ERROR, "BSSGP TLLI=0x%08x Rx UL-UD "</span><br><span> "missing mandatory IE\n", msgb_tlli(msg));</span><br><span>@@ -497,8 +497,8 @@</span><br><span> uint16_t ns_bvci = msgb_bvci(msg), nsei = msgb_nsei(msg);</span><br><span> int rc;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(tp, BSSGP_IE_TLLI) ||</span><br><span style="color: hsl(0, 100%, 40%);">- !TLVP_PRESENT(tp, BSSGP_IE_ROUTEING_AREA)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(tp, BSSGP_IE_TLLI, 4) ||</span><br><span style="color: hsl(120, 100%, 40%);">+ !TLVP_PRES_LEN(tp, BSSGP_IE_ROUTEING_AREA, 6)) {</span><br><span> LOGP(DBSSGP, LOGL_ERROR, "BSSGP BVCI=%u Rx SUSPEND "</span><br><span> "missing mandatory IE\n", ns_bvci);</span><br><span> return bssgp_tx_status(BSSGP_CAUSE_MISSING_MAND_IE, NULL, msg);</span><br><span>@@ -538,9 +538,9 @@</span><br><span> uint16_t ns_bvci = msgb_bvci(msg), nsei = msgb_nsei(msg);</span><br><span> int rc;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(tp, BSSGP_IE_TLLI) ||</span><br><span style="color: hsl(0, 100%, 40%);">- !TLVP_PRESENT(tp, BSSGP_IE_ROUTEING_AREA) ||</span><br><span style="color: hsl(0, 100%, 40%);">- !TLVP_PRESENT(tp, BSSGP_IE_SUSPEND_REF_NR)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(tp, BSSGP_IE_TLLI, 4 ) ||</span><br><span style="color: hsl(120, 100%, 40%);">+ !TLVP_PRES_LEN(tp, BSSGP_IE_ROUTEING_AREA, 6) ||</span><br><span style="color: hsl(120, 100%, 40%);">+ !TLVP_PRES_LEN(tp, BSSGP_IE_SUSPEND_REF_NR, 1)) {</span><br><span> LOGP(DBSSGP, LOGL_ERROR, "BSSGP BVCI=%u Rx RESUME "</span><br><span> "missing mandatory IE\n", ns_bvci);</span><br><span> return bssgp_tx_status(BSSGP_CAUSE_MISSING_MAND_IE, NULL, msg);</span><br><span>@@ -580,16 +580,16 @@</span><br><span> uint32_t tlli = 0;</span><br><span> uint16_t nsei = msgb_nsei(msg);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(tp, BSSGP_IE_TLLI) ||</span><br><span style="color: hsl(0, 100%, 40%);">- !TLVP_PRESENT(tp, BSSGP_IE_LLC_FRAMES_DISCARDED) ||</span><br><span style="color: hsl(0, 100%, 40%);">- !TLVP_PRESENT(tp, BSSGP_IE_BVCI) ||</span><br><span style="color: hsl(0, 100%, 40%);">- !TLVP_PRESENT(tp, BSSGP_IE_NUM_OCT_AFF)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(tp, BSSGP_IE_TLLI, 4) ||</span><br><span style="color: hsl(120, 100%, 40%);">+ !TLVP_PRES_LEN(tp, BSSGP_IE_LLC_FRAMES_DISCARDED, 1) ||</span><br><span style="color: hsl(120, 100%, 40%);">+ !TLVP_PRES_LEN(tp, BSSGP_IE_BVCI, 2) ||</span><br><span style="color: hsl(120, 100%, 40%);">+ !TLVP_PRES_LEN(tp, BSSGP_IE_NUM_OCT_AFF, 3)) {</span><br><span> LOGP(DBSSGP, LOGL_ERROR, "BSSGP BVCI=%u Rx LLC DISCARDED "</span><br><span> "missing mandatory IE\n", ctx->bvci);</span><br><span style="color: hsl(120, 100%, 40%);">+ return bssgp_tx_status(BSSGP_CAUSE_MISSING_MAND_IE, NULL, msg);</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (TLVP_PRESENT(tp, BSSGP_IE_TLLI))</span><br><span style="color: hsl(0, 100%, 40%);">- tlli = tlvp_val32be(tp, BSSGP_IE_TLLI);</span><br><span style="color: hsl(120, 100%, 40%);">+ tlli = tlvp_val32be(tp, BSSGP_IE_TLLI);</span><br><span> </span><br><span> DEBUGP(DBSSGP, "BSSGP BVCI=%u TLLI=%08x Rx LLC DISCARDED\n",</span><br><span> ctx->bvci, tlli);</span><br><span>@@ -615,7 +615,7 @@</span><br><span> struct osmo_bssgp_prim nmp;</span><br><span> enum gprs_bssgp_cause cause;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(tp, BSSGP_IE_CAUSE)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(tp, BSSGP_IE_CAUSE, 1)) {</span><br><span> LOGP(DBSSGP, LOGL_ERROR, "BSSGP BVCI=%u Rx STATUS "</span><br><span> "missing mandatory IE\n", bvci);</span><br><span> cause = BSSGP_CAUSE_PROTO_ERR_UNSPEC;</span><br><span>@@ -627,7 +627,7 @@</span><br><span> bvci, bssgp_cause_str(cause));</span><br><span> </span><br><span> if (cause == BSSGP_CAUSE_BVCI_BLOCKED || cause == BSSGP_CAUSE_UNKNOWN_BVCI) {</span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(tp, BSSGP_IE_BVCI))</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(tp, BSSGP_IE_BVCI, 2))</span><br><span> LOGP(DBSSGP, LOGL_ERROR,</span><br><span> "BSSGP BVCI=%u Rx STATUS cause=%s "</span><br><span> "missing conditional BVCI IE\n",</span><br><span>@@ -882,11 +882,11 @@</span><br><span> DEBUGP(DBSSGP, "BSSGP BVCI=%u Rx Flow Control BVC\n",</span><br><span> bctx->bvci);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(tp, BSSGP_IE_TAG) ||</span><br><span style="color: hsl(0, 100%, 40%);">- !TLVP_PRESENT(tp, BSSGP_IE_BVC_BUCKET_SIZE) ||</span><br><span style="color: hsl(0, 100%, 40%);">- !TLVP_PRESENT(tp, BSSGP_IE_BUCKET_LEAK_RATE) ||</span><br><span style="color: hsl(0, 100%, 40%);">- !TLVP_PRESENT(tp, BSSGP_IE_BMAX_DEFAULT_MS) ||</span><br><span style="color: hsl(0, 100%, 40%);">- !TLVP_PRESENT(tp, BSSGP_IE_R_DEFAULT_MS)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(tp, BSSGP_IE_TAG, 1) ||</span><br><span style="color: hsl(120, 100%, 40%);">+ !TLVP_PRES_LEN(tp, BSSGP_IE_BVC_BUCKET_SIZE, 2) ||</span><br><span style="color: hsl(120, 100%, 40%);">+ !TLVP_PRES_LEN(tp, BSSGP_IE_BUCKET_LEAK_RATE, 2) ||</span><br><span style="color: hsl(120, 100%, 40%);">+ !TLVP_PRES_LEN(tp, BSSGP_IE_BMAX_DEFAULT_MS, 2) ||</span><br><span style="color: hsl(120, 100%, 40%);">+ !TLVP_PRES_LEN(tp, BSSGP_IE_R_DEFAULT_MS,2)) {</span><br><span> LOGP(DBSSGP, LOGL_ERROR, "BSSGP BVCI=%u Rx FC BVC "</span><br><span> "missing mandatory IE\n", bctx->bvci);</span><br><span> return bssgp_tx_status(BSSGP_CAUSE_MISSING_MAND_IE, NULL, msg);</span><br><span>@@ -1040,8 +1040,8 @@</span><br><span> break;</span><br><span> case BSSGP_PDUT_BVC_BLOCK:</span><br><span> /* BSS tells us that BVC shall be blocked */</span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(tp, BSSGP_IE_BVCI) ||</span><br><span style="color: hsl(0, 100%, 40%);">- !TLVP_PRESENT(tp, BSSGP_IE_CAUSE)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(tp, BSSGP_IE_BVCI, 2) ||</span><br><span style="color: hsl(120, 100%, 40%);">+ !TLVP_PRES_LEN(tp, BSSGP_IE_CAUSE, 1)) {</span><br><span> LOGP(DBSSGP, LOGL_ERROR, "BSSGP Rx BVC-BLOCK "</span><br><span> "missing mandatory IE\n");</span><br><span> goto err_mand_ie;</span><br><span>@@ -1050,7 +1050,7 @@</span><br><span> break;</span><br><span> case BSSGP_PDUT_BVC_UNBLOCK:</span><br><span> /* BSS tells us that BVC shall be unblocked */</span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(tp, BSSGP_IE_BVCI)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(tp, BSSGP_IE_BVCI, 2)) {</span><br><span> LOGP(DBSSGP, LOGL_ERROR, "BSSGP Rx BVC-UNBLOCK "</span><br><span> "missing mandatory IE\n");</span><br><span> goto err_mand_ie;</span><br><span>@@ -1062,8 +1062,8 @@</span><br><span> break;</span><br><span> case BSSGP_PDUT_BVC_RESET:</span><br><span> /* BSS tells us that BVC init is required */</span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(tp, BSSGP_IE_BVCI) ||</span><br><span style="color: hsl(0, 100%, 40%);">- !TLVP_PRESENT(tp, BSSGP_IE_CAUSE)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(tp, BSSGP_IE_BVCI, 2) ||</span><br><span style="color: hsl(120, 100%, 40%);">+ !TLVP_PRES_LEN(tp, BSSGP_IE_CAUSE, 1)) {</span><br><span> LOGP(DBSSGP, LOGL_ERROR, "BSSGP Rx BVC-RESET "</span><br><span> "missing mandatory IE\n");</span><br><span> goto err_mand_ie;</span><br><span>@@ -1135,7 +1135,7 @@</span><br><span> return rc;</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- if (bvci == BVCI_SIGNALLING && TLVP_PRESENT(&tp, BSSGP_IE_BVCI))</span><br><span style="color: hsl(120, 100%, 40%);">+ if (bvci == BVCI_SIGNALLING && TLVP_PRES_LEN(&tp, BSSGP_IE_BVCI, 2))</span><br><span> bvci = tlvp_val16be(&tp, BSSGP_IE_BVCI);</span><br><span> </span><br><span> /* look-up or create the BTS context for this BVC */</span><br><span>diff --git a/src/gb/gprs_bssgp_bss.c b/src/gb/gprs_bssgp_bss.c</span><br><span>index 59b06f0..462666a 100644</span><br><span>--- a/src/gb/gprs_bssgp_bss.c</span><br><span>+++ b/src/gb/gprs_bssgp_bss.c</span><br><span>@@ -511,24 +511,24 @@</span><br><span> TLVP_LEN(&tp, BSSGP_IE_IMSI));</span><br><span> </span><br><span> /* DRX Parameters */</span><br><span style="color: hsl(0, 100%, 40%);">- if (!TLVP_PRESENT(&tp, BSSGP_IE_DRX_PARAMS))</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!TLVP_PRES_LEN(&tp, BSSGP_IE_DRX_PARAMS, 2))</span><br><span> goto err_mand_ie;</span><br><span> pinfo->drx_params = tlvp_val16be(&tp, BSSGP_IE_DRX_PARAMS);</span><br><span> </span><br><span> /* Scope */</span><br><span style="color: hsl(0, 100%, 40%);">- if (TLVP_PRESENT(&tp, BSSGP_IE_BSS_AREA_ID)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (TLVP_PRES_LEN(&tp, BSSGP_IE_BSS_AREA_ID, 1)) {</span><br><span> pinfo->scope = BSSGP_PAGING_BSS_AREA;</span><br><span style="color: hsl(0, 100%, 40%);">- } else if (TLVP_PRESENT(&tp, BSSGP_IE_LOCATION_AREA)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ } else if (TLVP_PRES_LEN(&tp, BSSGP_IE_LOCATION_AREA, 5)) {</span><br><span> pinfo->scope = BSSGP_PAGING_LOCATION_AREA;</span><br><span> memcpy(ra, TLVP_VAL(&tp, BSSGP_IE_LOCATION_AREA),</span><br><span> TLVP_LEN(&tp, BSSGP_IE_LOCATION_AREA));</span><br><span> gsm48_parse_ra(&pinfo->raid, ra);</span><br><span style="color: hsl(0, 100%, 40%);">- } else if (TLVP_PRESENT(&tp, BSSGP_IE_ROUTEING_AREA)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ } else if (TLVP_PRES_LEN(&tp, BSSGP_IE_ROUTEING_AREA, 6)) {</span><br><span> pinfo->scope = BSSGP_PAGING_ROUTEING_AREA;</span><br><span> memcpy(ra, TLVP_VAL(&tp, BSSGP_IE_ROUTEING_AREA),</span><br><span> TLVP_LEN(&tp, BSSGP_IE_ROUTEING_AREA));</span><br><span> gsm48_parse_ra(&pinfo->raid, ra);</span><br><span style="color: hsl(0, 100%, 40%);">- } else if (TLVP_PRESENT(&tp, BSSGP_IE_BVCI)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ } else if (TLVP_PRES_LEN(&tp, BSSGP_IE_BVCI, 2)) {</span><br><span> pinfo->scope = BSSGP_PAGING_BVCI;</span><br><span> pinfo->bvci = tlvp_val16be(&tp, BSSGP_IE_BVCI);</span><br><span> } else</span><br><span>@@ -546,8 +546,7 @@</span><br><span> }</span><br><span> </span><br><span> /* Optional (P-)TMSI */</span><br><span style="color: hsl(0, 100%, 40%);">- if (TLVP_PRESENT(&tp, BSSGP_IE_TMSI) &&</span><br><span style="color: hsl(0, 100%, 40%);">- TLVP_LEN(&tp, BSSGP_IE_TMSI) >= 4) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (TLVP_PRES_LEN(&tp, BSSGP_IE_TMSI, 4)) {</span><br><span> if (!pinfo->ptmsi)</span><br><span> pinfo->ptmsi = talloc_zero_size(pinfo, sizeof(uint32_t));</span><br><span> *(pinfo->ptmsi) = osmo_load32be(TLVP_VAL(&tp, BSSGP_IE_TMSI));</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.osmocom.org/c/libosmocore/+/21493">change 21493</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/libosmocore/+/21493"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: libosmocore </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I56e8b31ce51602d2681e3db501c48f84bfe7e438 </div>
<div style="display:none"> Gerrit-Change-Number: 21493 </div>
<div style="display:none"> Gerrit-PatchSet: 6 </div>
<div style="display:none"> Gerrit-Owner: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: daniel <dwillmann@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: merged </div>