From 5a513317292280aad8b9f81e1e1159153bd5ed4d Mon Sep 17 00:00:00 2001 From: Vadim Yanitskiy Date: Wed, 14 Dec 2022 00:23:59 +0700 Subject: [PATCH] gsm0808: add gsm0808_enc_speech_codec[_list]2() The problem with most of the existing gsm0808_* functions in this file is that they assert() too much, assuming that their callers always pass perfectly valid input parameters. But this is impossible on practice, as there can be bugs in complex projects using them, liks osmo-bsc. It was reported by a customer that a heavily loaded osmo-bsc crashed a few times, dropping more than 100 sites without network coverage for a few minutes. As was revealed during the investigaion, it crashed due to a failing assert at the end of enc_speech_codec(): OSMO_ASSERT(sc->cfg == 0); The problem is that somehow osmo-bsc is passing an unexpected sc->cfg value to gsm0808_create_ass_compl2(), in particular 0x02, while the given sc->type value (GSM0808_SCT_HR1) implies that there cannot be any configuration bits on the wire. The reason why and under which circumstances this can be happening is not clear yet, but what we agreed on so far is that the library API should be enforcing correctness of the input parameters in a less agressive way, rather than aborting the process without letting it any chance to recover. Modify the original gsm0808_enc_speech_codec[_list]() functions, so that a negative value is returned in case of an error. Rename them and add backwards compatibility wrappers, because it's public API. A separate patch making use of the new API follows next. Change-Id: I199ffa0ba4a64813238519178155dfc767aa3975 Related: SYS#6229 --- include/osmocom/gsm/gsm0808_utils.h | 11 +++-- src/gsm/gsm0808_utils.c | 73 ++++++++++++++++++++--------- src/gsm/libosmogsm.map | 2 + 3 files changed, 61 insertions(+), 25 deletions(-) diff --git a/include/osmocom/gsm/gsm0808_utils.h b/include/osmocom/gsm/gsm0808_utils.h index 6abfeecc4..18f3ebe5f 100644 --- a/include/osmocom/gsm/gsm0808_utils.h +++ b/include/osmocom/gsm/gsm0808_utils.h @@ -111,12 +111,17 @@ uint8_t gsm0808_enc_lcls(struct msgb *msg, const struct osmo_lcls *lcls); int gsm0808_dec_lcls(struct osmo_lcls *lcls, const struct tlv_parsed *tp); uint8_t gsm0808_enc_speech_codec(struct msgb *msg, - const struct gsm0808_speech_codec *sc); + const struct gsm0808_speech_codec *sc) + OSMO_DEPRECATED("use gsm0808_enc_speech_codec2() instead"); +int gsm0808_enc_speech_codec2(struct msgb *msg, + const struct gsm0808_speech_codec *sc); int gsm0808_dec_speech_codec(struct gsm0808_speech_codec *sc, const uint8_t *elem, uint8_t len); uint8_t gsm0808_enc_speech_codec_list(struct msgb *msg, - const struct gsm0808_speech_codec_list - *scl); + const struct gsm0808_speech_codec_list *scl) + OSMO_DEPRECATED("use gsm0808_enc_speech_codec_list2() instead"); +int gsm0808_enc_speech_codec_list2(struct msgb *msg, + const struct gsm0808_speech_codec_list *scl); int gsm0808_dec_speech_codec_list(struct gsm0808_speech_codec_list *scl, const uint8_t *elem, uint8_t len); uint8_t gsm0808_enc_channel_type(struct msgb *msg, diff --git a/src/gsm/gsm0808_utils.c b/src/gsm/gsm0808_utils.c index a1f51eb48..9d787f26f 100644 --- a/src/gsm/gsm0808_utils.c +++ b/src/gsm/gsm0808_utils.c @@ -209,10 +209,10 @@ static void decode_lai(const uint8_t *data, struct osmo_location_area_id *decode gsm48_decode_lai2(&lai, decoded); } -/* Helper function for gsm0808_enc_speech_codec() - * and gsm0808_enc_speech_codec_list() */ -static uint8_t enc_speech_codec(struct msgb *msg, - const struct gsm0808_speech_codec *sc) +/* Helper function for gsm0808_enc_speech_codec[_list](). + * Returns number of bytes appended; negative on error. */ +static int enc_speech_codec(struct msgb *msg, + const struct gsm0808_speech_codec *sc) { /* See also 3GPP TS 48.008 3.2.2.103 Speech Codec List */ uint8_t header = 0; @@ -240,8 +240,7 @@ static uint8_t enc_speech_codec(struct msgb *msg, break; default: /* Invalid codec type specified */ - OSMO_ASSERT(false); - break; + return -EINVAL; } old_tail = msg->tail; @@ -277,11 +276,13 @@ static uint8_t enc_speech_codec(struct msgb *msg, case GSM0808_SCT_FR5: case GSM0808_SCT_HR4: case GSM0808_SCT_CSD: - OSMO_ASSERT((sc->cfg & 0xff00) == 0); + if (sc->cfg >> 8) + return -EINVAL; msgb_put_u8(msg, (uint8_t) sc->cfg & 0xff); break; default: - OSMO_ASSERT(sc->cfg == 0); + if (sc->cfg != 0) + return -EINVAL; break; } @@ -291,24 +292,38 @@ static uint8_t enc_speech_codec(struct msgb *msg, /*! Encode TS 08.08 Speech Codec IE * \param[out] msg Message Buffer to which IE will be appended * \param[in] sc Speech Codec to be encoded into IE - * \returns number of bytes appended to \a msg */ -uint8_t gsm0808_enc_speech_codec(struct msgb *msg, - const struct gsm0808_speech_codec *sc) + * \returns number of bytes appended to \a msg; negative on error */ +int gsm0808_enc_speech_codec2(struct msgb *msg, + const struct gsm0808_speech_codec *sc) { /*! See also 3GPP TS 48.008 3.2.2.103 Speech Codec List */ - uint8_t *old_tail; uint8_t *tlv_len; + int rc; msgb_put_u8(msg, GSM0808_IE_SPEECH_CODEC); tlv_len = msgb_put(msg, 1); - old_tail = msg->tail; - enc_speech_codec(msg, sc); + rc = enc_speech_codec(msg, sc); + if (rc < 0) + return rc; - *tlv_len = (uint8_t) (msg->tail - old_tail); + *tlv_len = rc; return *tlv_len + 2; } +/*! Deprecated: gsm0808_enc_speech_codec2() wrapper for backwards compatibility. + * Mimics the old behavior: crash on missing and/or invalid input. */ +uint8_t gsm0808_enc_speech_codec(struct msgb *msg, + const struct gsm0808_speech_codec *sc) +{ + int rc; + + rc = gsm0808_enc_speech_codec2(msg, sc); + OSMO_ASSERT(rc > 0); + + return rc; +} + /*! Decode TS 08.08 Speech Codec IE * \param[out] sc Caller-allocated memory for Speech Codec * \param[in] elem IE value to be decoded @@ -392,15 +407,14 @@ int gsm0808_dec_speech_codec(struct gsm0808_speech_codec *sc, /*! Encode TS 08.08 Speech Codec list * \param[out] msg Message Buffer to which IE is to be appended * \param[in] scl Speech Codec List to be encoded into IE - * \returns number of bytes added to \a msg */ -uint8_t gsm0808_enc_speech_codec_list(struct msgb *msg, - const struct gsm0808_speech_codec_list *scl) + * \returns number of bytes added to \a msg; negative on error */ +int gsm0808_enc_speech_codec_list2(struct msgb *msg, + const struct gsm0808_speech_codec_list *scl) { /*! See also 3GPP TS 48.008 3.2.2.103 Speech Codec List */ uint8_t *old_tail; uint8_t *tlv_len; unsigned int i; - uint8_t rc; unsigned int bytes_used = 0; msgb_put_u8(msg, GSM0808_IE_SPEECH_CODEC_LIST); @@ -408,16 +422,31 @@ uint8_t gsm0808_enc_speech_codec_list(struct msgb *msg, old_tail = msg->tail; for (i = 0; i < scl->len; i++) { - rc = enc_speech_codec(msg, &scl->codec[i]); - OSMO_ASSERT(rc >= 1); + int rc = enc_speech_codec(msg, &scl->codec[i]); + if (rc < 1) + return rc; bytes_used += rc; - OSMO_ASSERT(bytes_used <= 255); + if (bytes_used > 0xff) + return -EOVERFLOW; } *tlv_len = (uint8_t) (msg->tail - old_tail); return *tlv_len + 2; } +/*! Deprecated: gsm0808_enc_speech_codec_list2() wrapper for backwards compatibility. + * Mimics the old behavior: crash on missing and/or invalid input. */ +uint8_t gsm0808_enc_speech_codec_list(struct msgb *msg, + const struct gsm0808_speech_codec_list *scl) +{ + int rc; + + rc = gsm0808_enc_speech_codec_list2(msg, scl); + OSMO_ASSERT(rc > 0); + + return rc; +} + /*! Decode TS 08.08 Speech Codec list IE * \param[out] scl Caller-provided memory to store codec list * \param[in] elem IE value to be decoded diff --git a/src/gsm/libosmogsm.map b/src/gsm/libosmogsm.map index b971ca016..7b5cd94e7 100644 --- a/src/gsm/libosmogsm.map +++ b/src/gsm/libosmogsm.map @@ -212,8 +212,10 @@ gsm0808_enc_aoip_trasp_addr; gsm0808_dec_aoip_trasp_addr; gsm0808_dec_osmux_cid; gsm0808_enc_speech_codec; +gsm0808_enc_speech_codec2; gsm0808_dec_speech_codec; gsm0808_enc_speech_codec_list; +gsm0808_enc_speech_codec_list2; gsm0808_dec_speech_codec_list; gsm0808_enc_channel_type; gsm0808_dec_channel_type;