Change in osmocom-bb[master]: trxcon/scheduler: fix: check primitive len before encoding

[Vadim Yanitskiy]

Vadim Yanitskiy has uploaded this change for review. ( https://gerrit.osmocom.org/10137


Change subject: trxcon/scheduler: fix: check primitive len before encoding
......................................................................

trxcon/scheduler: fix: check primitive len before encoding

We used to trust (and still doing this) the messages coming from
L1CTL interface too much, and not to check the primitive length
before passing the payload to the libosmocoding API. As was
discovered and described in OS#3415, sending a L1CTL message
(either DATA_REQ, or TRAFFIC_REQ) with an incorrect length
(lower than expected) may cause heap overflow.

Let's explicitly check a primitive before encoding, and drop it
if its length doesn't match the expected value(s).

Change-Id: I258ee9f6d0124b183b1db23a73f1e523fcea89a8
Fixes: OS#3415
---
M src/host/trxcon/sched_lchan_rach.c
M src/host/trxcon/sched_lchan_tchf.c
M src/host/trxcon/sched_lchan_xcch.c
3 files changed, 29 insertions(+), 3 deletions(-)



  git pull ssh://gerrit.osmocom.org:29418/osmocom-bb refs/changes/37/10137/1

diff --git a/src/host/trxcon/sched_lchan_rach.c b/src/host/trxcon/sched_lchan_rach.c
index 3f6e2e1..a255efb 100644
--- a/src/host/trxcon/sched_lchan_rach.c
+++ b/src/host/trxcon/sched_lchan_rach.c
@@ -69,6 +69,15 @@
 	uint8_t payload[36];
 	int rc;
 
+	/* Check the prim payload length */
+	if (lchan->prim->payload_len != sizeof(*req)) {
+		LOGP(DSCHD, LOGL_ERROR, "Primitive has odd length %zu (expected %zu), "
+			"so dropping...\n", lchan->prim->payload_len, sizeof(*req));
+
+		sched_prim_drop(lchan);
+		return -EINVAL;
+	}
+
 	/* Get the payload from a current primitive */
 	req = (struct l1ctl_rach_req *) lchan->prim->payload;
 
diff --git a/src/host/trxcon/sched_lchan_tchf.c b/src/host/trxcon/sched_lchan_tchf.c
index 80e4d52..e20b461 100644
--- a/src/host/trxcon/sched_lchan_tchf.c
+++ b/src/host/trxcon/sched_lchan_tchf.c
@@ -232,9 +232,17 @@
 		return -EINVAL;
 	}
 
-	/* Determine payload length */
-	if (lchan->prim->payload_len == GSM_MACBLOCK_LEN)
-		l2_len = GSM_MACBLOCK_LEN;
+	/* Determine and check the payload length */
+	if (lchan->prim->payload_len == GSM_MACBLOCK_LEN) {
+		l2_len = GSM_MACBLOCK_LEN; /* FACCH */
+	} else if (lchan->prim->payload_len != l2_len) {
+		LOGP(DSCHD, LOGL_ERROR, "Primitive has odd length %zu "
+			"(expected %zu for TCH or %u for FACCH), so dropping...\n",
+			lchan->prim->payload_len, l2_len, GSM_MACBLOCK_LEN);
+
+		sched_prim_drop(lchan);
+		return -EINVAL;
+	}
 
 	/* Shift buffer by 4 bursts back for interleaving */
 	memcpy(buffer, buffer + 464, 464);
diff --git a/src/host/trxcon/sched_lchan_xcch.c b/src/host/trxcon/sched_lchan_xcch.c
index 04c9f4e..b31f727 100644
--- a/src/host/trxcon/sched_lchan_xcch.c
+++ b/src/host/trxcon/sched_lchan_xcch.c
@@ -141,6 +141,15 @@
 			return 0;
 	}
 
+	/* Check the prim payload length */
+	if (lchan->prim->payload_len != GSM_MACBLOCK_LEN) {
+		LOGP(DSCHD, LOGL_ERROR, "Primitive has odd length %zu (expected %u), "
+			"so dropping...\n", lchan->prim->payload_len, GSM_MACBLOCK_LEN);
+
+		sched_prim_drop(lchan);
+		return -EINVAL;
+	}
+
 	/* Encode payload */
 	rc = gsm0503_xcch_encode(buffer, lchan->prim->payload);
 	if (rc) {

-- 
To view, visit https://gerrit.osmocom.org/10137
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I258ee9f6d0124b183b1db23a73f1e523fcea89a8
Gerrit-Change-Number: 10137
Gerrit-PatchSet: 1
Gerrit-Owner: Vadim Yanitskiy <axilirator at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20180724/43b083fb/attachment.htm>