Currently the implementation of bssgp_tx_dl_ud conditionally adds some optional IE if dup != NULL. Later on is dereferences dup to access qos_profile and fc, but this without checking dup in advance. This may lead to an segmentation violation fault.
This commit changes the value range of the function to only accept dup != NULL. An assertion will fail otherwise. All other explicit checks for non-NULL are removed.
Fixes: Coverity CID 1040673 Sponsored-by: On-Waves ehf --- src/gb/gprs_bssgp.c | 49 ++++++++++++++++++++++++------------------------- 1 file changed, 24 insertions(+), 25 deletions(-)
diff --git a/src/gb/gprs_bssgp.c b/src/gb/gprs_bssgp.c index 2f23290..a3fd6aa 100644 --- a/src/gb/gprs_bssgp.c +++ b/src/gb/gprs_bssgp.c @@ -1092,6 +1092,8 @@ int bssgp_tx_dl_ud(struct msgb *msg, uint16_t pdu_lifetime, uint16_t _pdu_lifetime = htons(pdu_lifetime); /* centi-seconds */ uint16_t drx_params;
+ OSMO_ASSERT(dup != NULL); + /* Identifiers from UP: TLLI, BVCI, NSEI (all in msgb->cb) */ if (bvci <= BVCI_PTM ) { LOGP(DBSSGP, LOGL_ERROR, "Cannot send DL-UD to BVCI %u\n", @@ -1124,35 +1126,32 @@ int bssgp_tx_dl_ud(struct msgb *msg, uint16_t pdu_lifetime,
/* FIXME: optional elements: Alignment, UTRAN CCO, LSA, PFI */
- if (dup) { - /* Old TLLI to help BSS map from old->new */ - if (dup->tlli) { - uint32_t tlli = htonl(*dup->tlli); - msgb_tvlv_push(msg, BSSGP_IE_TLLI, 4, (uint8_t *) &tlli); - } - - /* IMSI */ - if (dup->imsi && strlen(dup->imsi)) { - uint8_t mi[10]; - int imsi_len = gsm48_generate_mid_from_imsi(mi, dup->imsi); - if (imsi_len > 2) - msgb_tvlv_push(msg, BSSGP_IE_IMSI, - imsi_len-2, mi+2); - } + /* Old TLLI to help BSS map from old->new */ + if (dup->tlli) { + uint32_t tlli = htonl(*dup->tlli); + msgb_tvlv_push(msg, BSSGP_IE_TLLI, 4, (uint8_t *) &tlli); + }
- /* DRX parameters */ - drx_params = htons(dup->drx_parms); - msgb_tvlv_push(msg, BSSGP_IE_DRX_PARAMS, 2, - (uint8_t *) &drx_params); + /* IMSI */ + if (dup->imsi && strlen(dup->imsi)) { + uint8_t mi[10]; + int imsi_len = gsm48_generate_mid_from_imsi(mi, dup->imsi); + if (imsi_len > 2) + msgb_tvlv_push(msg, BSSGP_IE_IMSI, + imsi_len-2, mi+2); + }
- /* FIXME: Priority */ + /* DRX parameters */ + drx_params = htons(dup->drx_parms); + msgb_tvlv_push(msg, BSSGP_IE_DRX_PARAMS, 2, + (uint8_t *) &drx_params);
- /* MS Radio Access Capability */ - if (dup->ms_ra_cap.len) - msgb_tvlv_push(msg, BSSGP_IE_MS_RADIO_ACCESS_CAP, - dup->ms_ra_cap.len, dup->ms_ra_cap.v); + /* FIXME: Priority */
- } + /* MS Radio Access Capability */ + if (dup->ms_ra_cap.len) + msgb_tvlv_push(msg, BSSGP_IE_MS_RADIO_ACCESS_CAP, + dup->ms_ra_cap.len, dup->ms_ra_cap.v);
/* prepend the pdu lifetime */ msgb_tvlv_push(msg, BSSGP_IE_PDU_LIFETIME, 2, (uint8_t *)&_pdu_lifetime);
Currently bssgp_rx_ptp might be called with bctx being NULL, when the NS BVCI is neither BVCI_SIGNALLING nor BVCI_PTM, but the message is a BVC_RESET or it contains an BVCI IE != BVCI_SIGNALLING where the BVCI is not known.
This patch ensures that bssgp_rx_ptp will only be called with a non-NULL bctx. A log message will be issued, if the bctx is NULL when this was not expected.
Fixes: Coverity CID 1040674 Sponsored-by: On-Waves ehf --- src/gb/gprs_bssgp.c | 7 ++++++- tests/gb/gprs_bssgp_test.c | 17 +++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/src/gb/gprs_bssgp.c b/src/gb/gprs_bssgp.c index a3fd6aa..4c93b69 100644 --- a/src/gb/gprs_bssgp.c +++ b/src/gb/gprs_bssgp.c @@ -1073,8 +1073,13 @@ int bssgp_rcvmsg(struct msgb *msg) rc = bssgp_rx_sign(msg, &tp, bctx); else if (ns_bvci == BVCI_PTM) rc = bssgp_tx_status(BSSGP_CAUSE_PDU_INCOMP_FEAT, NULL, msg); - else + else if (bctx) rc = bssgp_rx_ptp(msg, &tp, bctx); + else + LOGP(DBSSGP, LOGL_NOTICE, + "NSEI=%u/BVCI=%u Cannot handle PDU type %u for " + "unknown BVCI, NS BVCI %u\n", + msgb_nsei(msg), bvci, pdu_type, ns_bvci);
return rc; } diff --git a/tests/gb/gprs_bssgp_test.c b/tests/gb/gprs_bssgp_test.c index 3d1384b..b454430 100644 --- a/tests/gb/gprs_bssgp_test.c +++ b/tests/gb/gprs_bssgp_test.c @@ -159,6 +159,22 @@ static void test_bssgp_status(void) printf("----- %s END\n", __func__); }
+static void test_bssgp_bad_reset() +{ + struct msgb *msg = bssgp_msgb_alloc(); + uint16_t bvci_be = htons(2); + uint8_t cause = BSSGP_CAUSE_OML_INTERV; + + msgb_v_put(msg, BSSGP_PDUT_BVC_RESET); + msgb_tvlv_put(msg, BSSGP_IE_BVCI, sizeof(bvci_be), (uint8_t *)&bvci_be); + msgb_tvlv_put(msg, BSSGP_IE_CAUSE, sizeof(cause), &cause); + + msgb_bvci(msg) = 0xbad; + + msgb_bssgp_send_and_free(msg); +} + + static struct log_info info = {};
int main(int argc, char **argv) @@ -181,6 +197,7 @@ int main(int argc, char **argv) printf("===== BSSGP test START\n"); test_bssgp_suspend_resume(); test_bssgp_status(); + test_bssgp_bad_reset(); printf("===== BSSGP test END\n\n");
exit(EXIT_SUCCESS);
Currently the return value of the gprs_ns_tx family of functions is often ignored. This is not a serious issue, since the successful delivery of the messages is neither guaranteed nor acknowledged by the network layer anyway.
Nevertheless this commit adds logging (level INFO) to gprs_ns_tx and gprs_ns_msgb_alloc. The definition of the latter has been moved from the header file to gprs_ns.c.
Fixes: Coverity CID 1040678, 1040679, 1040680, 1040681, 1040682, 1040683, 1040684, 1040686, 1040687, 1040688, 1111545, 1240203, 1240204 Sponsored-by: On-Waves ehf --- include/osmocom/gprs/gprs_ns.h | 10 ++++------ src/gb/gprs_ns.c | 19 +++++++++++++++++++ src/gb/libosmogb.map | 1 + 3 files changed, 24 insertions(+), 6 deletions(-)
diff --git a/include/osmocom/gprs/gprs_ns.h b/include/osmocom/gprs/gprs_ns.h index e77ca42..d5a605d 100644 --- a/include/osmocom/gprs/gprs_ns.h +++ b/include/osmocom/gprs/gprs_ns.h @@ -23,6 +23,9 @@ "Alive Timer (Tns-alive) timeout\n" \ "Alive Timer (Tns-alive) number of retries\n"
+#define NS_ALLOC_SIZE 2048 +#define NS_ALLOC_HEADROOM 20 + enum ns_timeout { NS_TOUT_TNS_BLOCK, NS_TOUT_TNS_BLOCK_RETRIES, @@ -186,12 +189,7 @@ void gprs_ns_ll_copy(struct gprs_nsvc *nsvc, struct gprs_nsvc *other); /* Clear the link layer info (will never match a real link then) */ void gprs_ns_ll_clear(struct gprs_nsvc *nsvc);
-#define NS_ALLOC_SIZE 2048 -#define NS_ALLOC_HEADROOM 20 -static inline struct msgb *gprs_ns_msgb_alloc(void) -{ - return msgb_alloc_headroom(NS_ALLOC_SIZE, NS_ALLOC_HEADROOM, "GPRS/NS"); -} +struct msgb *gprs_ns_msgb_alloc(void);
enum signal_ns { S_NS_RESET, diff --git a/src/gb/gprs_ns.c b/src/gb/gprs_ns.c index 27968e3..827d09d 100644 --- a/src/gb/gprs_ns.c +++ b/src/gb/gprs_ns.c @@ -131,6 +131,17 @@ static const struct rate_ctr_group_desc nsvc_ctrg_desc = { LOGP(DNS, LOGL_ERROR, "TX failed (%d) to peer %s\n", \ rc, gprs_ns_ll_str(nsvc));
+struct msgb *gprs_ns_msgb_alloc(void) +{ + struct msgb *msg = msgb_alloc_headroom(NS_ALLOC_SIZE, NS_ALLOC_HEADROOM, + "GPRS/NS"); + if (!msg) { + LOGP(DNS, LOGL_ERROR, "Failed to allocate NS message of size %d\n", + NS_ALLOC_SIZE); + } + return msg; +} +
/*! \brief Lookup struct gprs_nsvc based on NSVCI * \param[in] nsi NS instance in which to search @@ -298,9 +309,17 @@ static int gprs_ns_tx(struct gprs_nsvc *nsvc, struct msgb *msg) switch (nsvc->ll) { case GPRS_NS_LL_UDP: ret = nsip_sendmsg(nsvc, msg); + if (ret < 0) + LOGP(DNS, LOGL_INFO, + "failed to send NS message via UDP: %s\n", + strerror(-ret)); break; case GPRS_NS_LL_FR_GRE: ret = gprs_ns_frgre_sendmsg(nsvc, msg); + if (ret < 0) + LOGP(DNS, LOGL_INFO, + "failed to send NS message via FR/GRE: %s\n", + strerror(-ret)); break; default: LOGP(DNS, LOGL_ERROR, "unsupported NS linklayer %u\n", nsvc->ll); diff --git a/src/gb/libosmogb.map b/src/gb/libosmogb.map index a21a7ac..43ebbf8 100644 --- a/src/gb/libosmogb.map +++ b/src/gb/libosmogb.map @@ -57,6 +57,7 @@ gprs_ns_vty_init; gprs_ns_ll_str; gprs_ns_ll_copy; gprs_ns_ll_clear; +gprs_ns_msgb_alloc;
gprs_nsvc_create; gprs_nsvc_delete;
-
Jacob Erlbeck