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 <[email protected]>
