neels has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-bts/+/39379?usp=email ) Change subject: trx_data_read_cb(): make length checks waterproof ...................................................................... trx_data_read_cb(): make length checks waterproof Header lengths are defined in two places: - trx_data_rx_hdr_len[], - rc of trx_data_handle_*(). Before this patch, we used the one to validate the length, and the other to read the buffer data. Use only the trx_data_rx_hdr_len[] values for validation as well as reading. Make sure that any (successful) return values exactly match the header length that was valiated earlier. Use a separate rc variable (of the more fitting int type) for the function calls, and keep only one unchanging hdr_len from the start. Related: CID#465552 Change-Id: Ic658824a5884598d1245511897bcc00050c14317 --- M src/osmo-bts-trx/trx_if.c 1 file changed, 14 insertions(+), 7 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bts refs/changes/79/39379/1 diff --git a/src/osmo-bts-trx/trx_if.c b/src/osmo-bts-trx/trx_if.c index b6b20e9..4aeac6d 100644 --- a/src/osmo-bts-trx/trx_if.c +++ b/src/osmo-bts-trx/trx_if.c @@ -1023,6 +1023,7 @@ struct trx_ul_burst_ind bi; ssize_t hdr_len, buf_len; uint8_t pdu_ver; + int rc; buf_len = recv(ofd->fd, trx_data_buf, sizeof(trx_data_buf), 0); if (OSMO_UNLIKELY(buf_len <= 0)) { @@ -1053,23 +1054,24 @@ bi.flags = 0x00; /* Make sure that we have enough bytes to parse the header */ - if (OSMO_UNLIKELY(buf_len < trx_data_rx_hdr_len[pdu_ver])) { + hdr_len = trx_data_rx_hdr_len[pdu_ver]; + if (OSMO_UNLIKELY(buf_len < hdr_len)) { LOGPPHI(l1h->phy_inst, DTRX, LOGL_ERROR, "Rx malformed TRXDv%u PDU: len=%zd < expected %u\n", - pdu_ver, buf_len, trx_data_rx_hdr_len[pdu_ver]); + pdu_ver, buf_len, hdr_len); return -EINVAL; } /* Parse header depending on the PDU version */ switch (pdu_ver) { case 0: /* TRXDv0 */ - hdr_len = trx_data_handle_hdr_v0(l1h->phy_inst, &bi, buf, buf_len); + rc = trx_data_handle_hdr_v0(l1h->phy_inst, &bi, buf, buf_len); break; case 1: /* TRXDv1 */ - hdr_len = trx_data_handle_hdr_v1(l1h->phy_inst, &bi, buf, buf_len); + rc = trx_data_handle_hdr_v1(l1h->phy_inst, &bi, buf, buf_len); break; case 2: /* TRXDv2 */ - hdr_len = trx_data_handle_pdu_v2(l1h->phy_inst, &bi, buf, buf_len); + rc = trx_data_handle_pdu_v2(l1h->phy_inst, &bi, buf, buf_len); break; default: /* Shall not happen */ @@ -1077,8 +1079,13 @@ } /* Header parsing error */ - if (OSMO_UNLIKELY(hdr_len < 0)) - return hdr_len; + if (OSMO_UNLIKELY(rc < 0)) + return rc; + + /* CID#465552: make sure buf_len - hdr_len can never become negative. It is already implied, but the + * indirection of hdr_len returned by trx_data_handle_*() opens more bug vectors. So make sure the + * trx_data_handle_*() return value is identical to the initially validated length. */ + OSMO_ASSERT(rc == hdr_len); if (OSMO_UNLIKELY(bi.fn >= GSM_TDMA_HYPERFRAME)) { LOGPPHI(l1h->phy_inst, DTRX, LOGL_ERROR, -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/39379?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: Ic658824a5884598d1245511897bcc00050c14317 Gerrit-Change-Number: 39379 Gerrit-PatchSet: 1 Gerrit-Owner: neels <nhofmeyr@sysmocom.de>