[openbsc 1/3] libbsc, osmo-bsc{, _nat}: Prevent unaligned access when casting TLVP_VAL
foo = *((uintXX_t *) TLVP_VAL(...) can lead to unaligned access. To prevent that use tlvp_valXX_unal() to get the values. --- openbsc/src/libbsc/abis_rsl.c | 10 +++++----- openbsc/src/osmo-bsc/osmo_bsc_bssap.c | 2 +- openbsc/src/osmo-bsc_nat/bsc_mgcp_utils.c | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/openbsc/src/libbsc/abis_rsl.c b/openbsc/src/libbsc/abis_rsl.c index 748ab7e..d193078 100644 --- a/openbsc/src/libbsc/abis_rsl.c +++ b/openbsc/src/libbsc/abis_rsl.c @@ -1804,20 +1804,20 @@ static void ipac_parse_rtp(struct gsm_lchan *lchan, struct tlv_parsed *tv) uint16_t port, conn_id; if (TLVP_PRESENT(tv, RSL_IE_IPAC_LOCAL_IP)) { - ip.s_addr = *((uint32_t *) TLVP_VAL(tv, RSL_IE_IPAC_LOCAL_IP)); + ip.s_addr = tlvp_val32_unal(tv, RSL_IE_IPAC_LOCAL_IP); DEBUGPC(DRSL, "LOCAL_IP=%s ", inet_ntoa(ip)); lchan->abis_ip.bound_ip = ntohl(ip.s_addr); } if (TLVP_PRESENT(tv, RSL_IE_IPAC_LOCAL_PORT)) { - port = *((uint16_t *) TLVP_VAL(tv, RSL_IE_IPAC_LOCAL_PORT)); + port = tlvp_val16_unal(tv, RSL_IE_IPAC_LOCAL_PORT); port = ntohs(port); DEBUGPC(DRSL, "LOCAL_PORT=%u ", port); lchan->abis_ip.bound_port = port; } if (TLVP_PRESENT(tv, RSL_IE_IPAC_CONN_ID)) { - conn_id = *((uint16_t *) TLVP_VAL(tv, RSL_IE_IPAC_CONN_ID)); + conn_id = tlvp_val16_unal(tv, RSL_IE_IPAC_CONN_ID); conn_id = ntohs(conn_id); DEBUGPC(DRSL, "CON_ID=%u ", conn_id); lchan->abis_ip.conn_id = conn_id; @@ -1838,13 +1838,13 @@ static void ipac_parse_rtp(struct gsm_lchan *lchan, struct tlv_parsed *tv) } if (TLVP_PRESENT(tv, RSL_IE_IPAC_REMOTE_IP)) { - ip.s_addr = *((uint32_t *) TLVP_VAL(tv, RSL_IE_IPAC_REMOTE_IP)); + ip.s_addr = tlvp_val32_unal(tv, RSL_IE_IPAC_REMOTE_IP); DEBUGPC(DRSL, "REMOTE_IP=%s ", inet_ntoa(ip)); lchan->abis_ip.connect_ip = ntohl(ip.s_addr); } if (TLVP_PRESENT(tv, RSL_IE_IPAC_REMOTE_PORT)) { - port = *((uint16_t *) TLVP_VAL(tv, RSL_IE_IPAC_REMOTE_PORT)); + port = tlvp_val16_unal(tv, RSL_IE_IPAC_REMOTE_PORT); port = ntohs(port); DEBUGPC(DRSL, "REMOTE_PORT=%u ", port); lchan->abis_ip.connect_port = port; diff --git a/openbsc/src/osmo-bsc/osmo_bsc_bssap.c b/openbsc/src/osmo-bsc/osmo_bsc_bssap.c index c2c2417..f58d96f 100644 --- a/openbsc/src/osmo-bsc/osmo_bsc_bssap.c +++ b/openbsc/src/osmo-bsc/osmo_bsc_bssap.c @@ -135,7 +135,7 @@ static int bssmap_handle_paging(struct osmo_msc_data *msc, if (TLVP_PRESENT(&tp, GSM0808_IE_TMSI) && TLVP_LEN(&tp, GSM0808_IE_TMSI) == 4) { - tmsi = ntohl(*(uint32_t *) TLVP_VAL(&tp, GSM0808_IE_TMSI)); + tmsi = ntohl(tlvp_val32_unal(&tp, GSM0808_IE_TMSI)); } /* diff --git a/openbsc/src/osmo-bsc_nat/bsc_mgcp_utils.c b/openbsc/src/osmo-bsc_nat/bsc_mgcp_utils.c index 22b8a35..e13827b 100644 --- a/openbsc/src/osmo-bsc_nat/bsc_mgcp_utils.c +++ b/openbsc/src/osmo-bsc_nat/bsc_mgcp_utils.c @@ -191,7 +191,7 @@ int bsc_mgcp_assign_patch(struct nat_sccp_connection *con, struct msgb *msg) return -1; } - cic = ntohs(*(uint16_t *)TLVP_VAL(&tp, GSM0808_IE_CIRCUIT_IDENTITY_CODE)); + cic = ntohs(tlvp_val16_unal(&tp, GSM0808_IE_CIRCUIT_IDENTITY_CODE)); timeslot = cic & 0x1f; multiplex = (cic & ~0x1f) >> 5; -- 1.8.4.2
Make the llc_default_params structure from which data is initialized large enough. Otherwise address sanitizer complains with out-of-bounds reads. --- openbsc/src/gprs/gprs_llc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openbsc/src/gprs/gprs_llc.c b/openbsc/src/gprs/gprs_llc.c index 70fe9dd..e6b1f07 100644 --- a/openbsc/src/gprs/gprs_llc.c +++ b/openbsc/src/gprs/gprs_llc.c @@ -84,7 +84,7 @@ static int _bssgp_tx_dl_ud(struct msgb *msg, struct sgsn_mm_ctx *mmctx) /* Section 8.9.9 LLC layer parameter default values */ -static const struct gprs_llc_params llc_default_params[] = { +static const struct gprs_llc_params llc_default_params[NUM_SAPIS] = { [1] = { .t200_201 = 5, .n200 = 3, -- 1.8.4.2
Address sanitizer complains with a buffer overflow to the end of gsm_fr_map: ERROR: AddressSanitizer: global-buffer-overflow on address 0x00000044f76c at pc 0x43c0dd bp 0x7fff18389db0 sp 0x7fff18389da8 READ of size 1 at 0x00000044f76c thread T0 #0 0x43c0dc in trau_encode_fr /home/alphaone/scm/osmo/openbsc/openbsc/src/libtrau/trau_mux.c:441 #1 0x42fad6 in test_trau_fr_efr /home/alphaone/scm/osmo/openbsc/openbsc/tests/trau/trau_test.c:35 #2 0x4308f4 in main /home/alphaone/scm/osmo/openbsc/openbsc/tests/trau/trau_test.c:70 #3 0x7f96e8cf04bc (/lib64/libc.so.6+0x224bc) #4 0x42f7ec (/home/alphaone/scm/osmo/openbsc/openbsc/tests/trau/trau_test+0x42f7ec) 0x00000044f76c is located 52 bytes to the left of global variable 'c_bits_check_fr' from 'trau_mux.c' (0x44f7a0) of size 5 0x00000044f76c is located 0 bytes to the right of global variable 'gsm_fr_map' from 'trau_mux.c' (0x44f720) of size 76 SUMMARY: AddressSanitizer: global-buffer-overflow /home/alphaone/scm/osmo/openbsc/openbsc/src/libtrau/trau_mux.c:441 trau_encode_fr In the last iteration of the loop k is already set to the next element in gsm_fr_map which leads to an out-of-bounds read. Instead decrement k at the end of the loop and put the check before the data assignment. This is functionally equivalent as k is never < 0 initially. This happens in trau_decode_fr as well. --- openbsc/src/libtrau/trau_mux.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/openbsc/src/libtrau/trau_mux.c b/openbsc/src/libtrau/trau_mux.c index 4f159e4..9b93eda 100644 --- a/openbsc/src/libtrau/trau_mux.c +++ b/openbsc/src/libtrau/trau_mux.c @@ -234,13 +234,14 @@ struct msgb *trau_decode_fr(uint32_t callref, l = 0; /* counts element bits */ o = 0; /* offset input bits */ while (i < 260) { - data[j/8] |= (tf->d_bits[k+o] << (7-(j%8))); - if (--k < 0) { + if (k < 0) { o += gsm_fr_map[l]; k = gsm_fr_map[++l]-1; } + data[j/8] |= (tf->d_bits[k+o] << (7-(j%8))); i++; j++; + k--; } frame->msg_type = GSM_TCHF_FRAME; frame->callref = callref; @@ -435,16 +436,14 @@ void trau_encode_fr(struct decoded_trau_frame *tf, l = 0; /* counts element bits */ o = 0; /* offset output bits */ while (i < 260) { - tf->d_bits[k+o] = (data[j/8] >> (7-(j%8))) & 1; - /* to avoid out-of-bounds access in gsm_fr_map[++l] */ - if (i == 259) - break; - if (--k < 0) { + if (k < 0) { o += gsm_fr_map[l]; k = gsm_fr_map[++l]-1; } + tf->d_bits[k+o] = (data[j/8] >> (7-(j%8))) & 1; i++; j++; + k--; } } -- 1.8.4.2
participants (1)
-
Daniel Willmann