7 Apr
2015
7 Apr
'15
5:52 p.m.
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);
--
1.9.1
7 Apr
7 Apr
5:52 p.m.
New subject: [PATCH 2/3] bssgp: Ensure non-NULL bctx before calling bssgp_rx_ptp (Coverity)
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);
--
1.9.1
5:52 p.m.
New subject: [PATCH 3/3] ns: Log when sending fails (Coverity)
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;
--
1.9.1
3316
days inactive
3316
days old
2 comments
1 participants
participants (1)
-
Jacob Erlbeck