falconia has uploaded this change for review.
refactor: replace rtppayload_is_valid() with preening before enqueue
Up until now, our approach to validating incoming RTP payloads and
dropping invalid ones has been to apply the preening function inside
l1sap_tch_rts_ind(), at the point of dequeueing from the DL input queue.
However, there are some RTP formats where we need to strip one byte
of header from the payload before passing the rest to our innards:
there is RFC 5993 for HR codec, and there also exists a non-standard
extension (rtp_traulike) that does a similar deal for FR and EFR.
Because of alignment issues, it will be much efficient (avoids memmove)
if we can do this header octet stripping before we copy the payload
into msgb - but doing so requires that we move this preening logic
to the point of RTP input before enqueueing. Make this change.
Related: OS#5688
Change-Id: I7fc99aeecba8303b56d397b8952de5eea82b301e
---
M include/osmo-bts/Makefile.am
A include/osmo-bts/rtp_input_preen.h
M src/common/Makefile.am
M src/common/l1sap.c
M src/common/osmux.c
A src/common/rtp_input_preen.c
6 files changed, 216 insertions(+), 72 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-bts refs/changes/88/32988/1
diff --git a/include/osmo-bts/Makefile.am b/include/osmo-bts/Makefile.am
index f24da53..0e45d70 100644
--- a/include/osmo-bts/Makefile.am
+++ b/include/osmo-bts/Makefile.am
@@ -12,6 +12,7 @@
oml.h \
paging.h \
rsl.h \
+ rtp_input_preen.h \
signal.h \
vty.h \
amr.h \
diff --git a/include/osmo-bts/rtp_input_preen.h b/include/osmo-bts/rtp_input_preen.h
new file mode 100644
index 0000000..3847845
--- /dev/null
+++ b/include/osmo-bts/rtp_input_preen.h
@@ -0,0 +1,20 @@
+/*
+ * A function common to RTP and Osmux input paths: validates incoming RTP
+ * payloads, makes the accept-or-drop decision, and for some codecs signals
+ * additional required actions such as dropping one header octet.
+ */
+
+#pragma once
+
+#include <stdint.h>
+#include <osmo-bts/lchan.h>
+
+enum pl_input_decision {
+ PL_DECISION_DROP,
+ PL_DECISION_ACCEPT_ASIS,
+ PL_DECISION_STRIP_HDR_OCTET,
+};
+
+enum pl_input_decision
+rtp_payload_input_preen(struct gsm_lchan *lchan, const uint8_t *rtp_pl,
+ unsigned rtp_pl_len);
diff --git a/src/common/Makefile.am b/src/common/Makefile.am
index 830f940..32f644c 100644
--- a/src/common/Makefile.am
+++ b/src/common/Makefile.am
@@ -36,6 +36,7 @@
bts_sm.c \
bts_trx.c \
rsl.c \
+ rtp_input_preen.c \
vty.c \
paging.c \
measurement.c \
diff --git a/src/common/l1sap.c b/src/common/l1sap.c
index f24bc2f..8142a86 100644
--- a/src/common/l1sap.c
+++ b/src/common/l1sap.c
@@ -54,6 +54,7 @@
#include <osmo-bts/bts_model.h>
#include <osmo-bts/handover.h>
#include <osmo-bts/msg_utils.h>
+#include <osmo-bts/rtp_input_preen.h>
#include <osmo-bts/pcuif_proto.h>
#include <osmo-bts/cbch.h>
@@ -1216,83 +1217,23 @@
return 1;
}
-static bool rtppayload_is_octet_aligned(const uint8_t *rtp_pl, uint8_t payload_len)
+/* This helper function for l1sap_tch_rts_ind() preens incoming RTP frames
+ * for FR/EFR SID: if the received frame is a valid SID per the rules of
+ * GSM 06.31/06.81 section 6.1.1, it needs to be rejuvenated by clearing
+ * all reserved bits and resetting the SID code word to error-free 95 zeros
+ * for FR or 95 ones for EFR, and if the received frame is an invalid SID
+ * per the same rules, then we need to drop it. We return false if
+ * l1sap_tch_rts_ind() needs to drop this frame, otherwise true. */
+static bool rtppayload_sid_preen(struct gsm_lchan *lchan, struct msgb *msg)
{
- /*
- * Logic: If 1st bit padding is not zero, packet is either:
- * - bandwidth-efficient AMR payload.
- * - malformed packet.
- * However, Bandwidth-efficient AMR 4,75 frame last in payload(F=0, FT=0)
- * with 4th,5ht,6th AMR payload to 0 matches padding==0.
- * Furthermore, both AMR 4,75 bw-efficient and octet alignment are 14 bytes long (AMR 4,75 encodes 95b):
- * bw-efficient: 95b, + 4b hdr + 6b ToC = 105b, + padding = 112b = 14B.
- * octet-aligned: 1B hdr + 1B ToC + 95b = 111b, + padding = 112b = 14B.
- * We cannot use other fields to match since they are inside the AMR
- * payload bits which are unknown.
- * As a result, this function may return false positive (true) for some AMR
- * 4,75 AMR frames, but given the length, CMR and FT read is the same as a
- * consequence, the damage in here is harmless other than being unable to
- * decode the audio at the other side.
- */
- #define AMR_PADDING1(rtp_pl) (rtp_pl[0] & 0x0f)
- #define AMR_PADDING2(rtp_pl) (rtp_pl[1] & 0x03)
-
- if (payload_len < 2 || AMR_PADDING1(rtp_pl) || AMR_PADDING2(rtp_pl))
- return false;
-
- return true;
-}
-
-static bool rtppayload_validate_fr(struct msgb *msg)
-{
- if (msg->len != GSM_FR_BYTES)
- return false;
- if ((msg->data[0] & 0xF0) != 0xD0)
- return false;
- return osmo_fr_sid_preen(msg->data);
-}
-
-static bool rtppayload_validate_efr(struct msgb *msg)
-{
- if (msg->len != GSM_EFR_BYTES)
- return false;
- if ((msg->data[0] & 0xF0) != 0xC0)
- return false;
- return osmo_efr_sid_preen(msg->data);
-}
-
-static bool rtppayload_is_valid(struct gsm_lchan *lchan, struct msgb *resp_msg)
-{
- /* If rtp continuous-streaming is enabled, we shall emit RTP packets
- * with zero-length payloads as BFI markers. In a TrFO scenario such
- * RTP packets sent by call leg A will be received by call leg B,
- * hence we need to handle them gracefully. For the purposes of a BTS
- * that runs on its own TDMA timing and does not need timing ticks from
- * an incoming RTP stream, the correct action upon receiving such
- * timing-tick-only RTP packets should be the same as when receiving
- * no RTP packet at all. The simplest way to produce that behavior
- * is to treat zero-length RTP payloads as invalid. */
- if (resp_msg->len == 0)
- return false;
-
switch (lchan->tch_mode) {
case GSM48_CMODE_SPEECH_V1:
if (lchan->type == GSM_LCHAN_TCH_F)
- return rtppayload_validate_fr(resp_msg);
+ return osmo_fr_sid_preen(msg->data);
else
- return true; /* FIXME: implement preening for HR1 */
+ return true; /* FIXME: see OS#6036 */
case GSM48_CMODE_SPEECH_EFR:
- return rtppayload_validate_efr(resp_msg);
- case GSM48_CMODE_SPEECH_AMR:
- /* Avoid forwarding bw-efficient AMR to lower layers,
- * most bts models don't support it. */
- if (!rtppayload_is_octet_aligned(resp_msg->data, resp_msg->len)) {
- LOGPLCHAN(lchan, DL1P, LOGL_NOTICE,
- "RTP->L1: Dropping unexpected AMR encoding (bw-efficient?) %s\n",
- osmo_hexdump(resp_msg->data, resp_msg->len));
- return false;
- }
- return true;
+ return osmo_efr_sid_preen(msg->data);
default:
return true;
}
@@ -1337,7 +1278,7 @@
if (!resp_msg) {
LOGPLCGT(lchan, &g_time, DL1P, LOGL_DEBUG, "DL TCH Tx queue underrun\n");
resp_l1sap = &empty_l1sap;
- } else if (!rtppayload_is_valid(lchan, resp_msg)) {
+ } else if (!rtppayload_sid_preen(lchan, resp_msg)) {
msgb_free(resp_msg);
resp_msg = NULL;
resp_l1sap = &empty_l1sap;
@@ -1943,6 +1884,20 @@
if (lchan->loopback)
return;
+ /* initial preen */
+ switch (rtp_payload_input_preen(lchan, rtp_pl, rtp_pl_len)) {
+ case PL_DECISION_DROP:
+ return;
+ case PL_DECISION_ACCEPT_ASIS:
+ break;
+ case PL_DECISION_STRIP_HDR_OCTET:
+ rtp_pl++;
+ rtp_pl_len--;
+ break;
+ default:
+ OSMO_ASSERT(0);
+ }
+
msg = l1sap_msgb_alloc(rtp_pl_len);
if (!msg)
return;
diff --git a/src/common/osmux.c b/src/common/osmux.c
index 8ca2b44..701e507 100644
--- a/src/common/osmux.c
+++ b/src/common/osmux.c
@@ -38,6 +38,7 @@
#include <osmo-bts/osmux.h>
#include <osmo-bts/lchan.h>
#include <osmo-bts/msg_utils.h>
+#include <osmo-bts/rtp_input_preen.h>
#include <osmo-bts/l1sap.h>
/* Bitmask containing Allocated Osmux circuit ID. +7 to round up to 8 bit boundary. */
@@ -409,6 +410,29 @@
*/
msgb_pull(msg, sizeof(struct osmo_phsap_prim));
+ /* We have to apply the same checks as in l1sap_rtp_rx_cb(), in case
+ * we got an invalid payload or a codec payload where we have to
+ * strip off one header byte. */
+ switch (rtp_payload_input_preen(lchan, msg->data, msg->len)) {
+ case PL_DECISION_DROP:
+ msgb_free(msg);
+ return;
+ case PL_DECISION_ACCEPT_ASIS:
+ break;
+ case PL_DECISION_STRIP_HDR_OCTET:
+ /* This case is messy: we have no choice other than memmove;
+ * if we were to do a simple msgb_pull(), the alignment will
+ * be off, and when l1sap_tch_rts_ind() later does its
+ * msgb_push() of l1sap header, that header will be
+ * misaligned, causing severe crippling effects at least
+ * on sysmoBTS ARM926. */
+ memmove(msg->data, msg->data + 1, msg->len - 1);
+ msgb_get(msg, 1);
+ break;
+ default:
+ OSMO_ASSERT(0);
+ }
+
/* enqueue making sure the queue doesn't get too long */
lchan_dl_tch_queue_enqueue(lchan, msg, 16);
}
diff --git a/src/common/rtp_input_preen.c b/src/common/rtp_input_preen.c
new file mode 100644
index 0000000..bb0ac11
--- /dev/null
+++ b/src/common/rtp_input_preen.c
@@ -0,0 +1,121 @@
+/*
+ * This module implements a function common to RTP and Osmux input paths:
+ * validates incoming RTP payloads, makes the accept-or-drop decision,
+ * and for some codecs signals additional required actions such as
+ * dropping one header octet.
+ *
+ * Author: Mychaela N. Falconia <falcon@freecalypso.org>, 2023 - however,
+ * Mother Mychaela's contributions are NOT subject to copyright.
+ * No rights reserved, all rights relinquished.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <stdbool.h>
+#include <stdint.h>
+
+#include <osmocom/core/logging.h>
+#include <osmocom/core/utils.h>
+
+#include <osmocom/codec/codec.h>
+
+#include <osmo-bts/lchan.h>
+#include <osmo-bts/logging.h>
+#include <osmo-bts/rtp_input_preen.h>
+
+static bool rtppayload_is_octet_aligned(const uint8_t *rtp_pl, unsigned payload_len)
+{
+ /*
+ * Logic: If 1st bit padding is not zero, packet is either:
+ * - bandwidth-efficient AMR payload.
+ * - malformed packet.
+ * However, Bandwidth-efficient AMR 4,75 frame last in payload(F=0, FT=0)
+ * with 4th,5ht,6th AMR payload to 0 matches padding==0.
+ * Furthermore, both AMR 4,75 bw-efficient and octet alignment are 14 bytes long (AMR 4,75 encodes 95b):
+ * bw-efficient: 95b, + 4b hdr + 6b ToC = 105b, + padding = 112b = 14B.
+ * octet-aligned: 1B hdr + 1B ToC + 95b = 111b, + padding = 112b = 14B.
+ * We cannot use other fields to match since they are inside the AMR
+ * payload bits which are unknown.
+ * As a result, this function may return false positive (true) for some AMR
+ * 4,75 AMR frames, but given the length, CMR and FT read is the same as a
+ * consequence, the damage in here is harmless other than being unable to
+ * decode the audio at the other side.
+ */
+ #define AMR_PADDING1(rtp_pl) (rtp_pl[0] & 0x0f)
+ #define AMR_PADDING2(rtp_pl) (rtp_pl[1] & 0x03)
+
+ if (payload_len < 2 || AMR_PADDING1(rtp_pl) || AMR_PADDING2(rtp_pl))
+ return false;
+
+ return true;
+}
+
+static enum pl_input_decision
+input_preen_fr(const uint8_t *rtp_pl, unsigned rtp_pl_len)
+{
+ if (rtp_pl_len != GSM_FR_BYTES)
+ return PL_DECISION_DROP;
+ if ((rtp_pl[0] & 0xF0) != 0xD0)
+ return PL_DECISION_DROP;
+ return PL_DECISION_ACCEPT_ASIS;
+}
+
+static enum pl_input_decision
+input_preen_efr(const uint8_t *rtp_pl, unsigned rtp_pl_len)
+{
+ if (rtp_pl_len != GSM_EFR_BYTES)
+ return PL_DECISION_DROP;
+ if ((rtp_pl[0] & 0xF0) != 0xC0)
+ return PL_DECISION_DROP;
+ return PL_DECISION_ACCEPT_ASIS;
+}
+
+enum pl_input_decision
+rtp_payload_input_preen(struct gsm_lchan *lchan, const uint8_t *rtp_pl,
+ unsigned rtp_pl_len)
+{
+ /* If rtp continuous-streaming is enabled, we shall emit RTP packets
+ * with zero-length payloads as BFI markers. In a TrFO scenario such
+ * RTP packets sent by call leg A will be received by call leg B,
+ * hence we need to handle them gracefully. For the purposes of a BTS
+ * that runs on its own TDMA timing and does not need timing ticks from
+ * an incoming RTP stream, the correct action upon receiving such
+ * timing-tick-only RTP packets should be the same as when receiving
+ * no RTP packet at all. The simplest way to produce that behavior
+ * is to treat zero-length RTP payloads as invalid. */
+ if (rtp_pl_len == 0)
+ return PL_DECISION_DROP;
+
+ switch (lchan->tch_mode) {
+ case GSM48_CMODE_SPEECH_V1:
+ if (lchan->type == GSM_LCHAN_TCH_F)
+ return input_preen_fr(rtp_pl, rtp_pl_len);
+ else
+ return PL_DECISION_ACCEPT_ASIS; /* FIXME: next patch in the series */
+ case GSM48_CMODE_SPEECH_EFR:
+ return input_preen_efr(rtp_pl, rtp_pl_len);
+ case GSM48_CMODE_SPEECH_AMR:
+ /* Avoid forwarding bw-efficient AMR to lower layers,
+ * most bts models don't support it. */
+ if (!rtppayload_is_octet_aligned(rtp_pl, rtp_pl_len)) {
+ LOGPLCHAN(lchan, DL1P, LOGL_NOTICE,
+ "RTP->L1: Dropping unexpected AMR encoding (bw-efficient?) %s\n",
+ osmo_hexdump(rtp_pl, rtp_pl_len));
+ return PL_DECISION_DROP;
+ }
+ return PL_DECISION_ACCEPT_ASIS;
+ default:
+ return PL_DECISION_ACCEPT_ASIS;
+ }
+}
To view, visit change 32988. To unsubscribe, or for help writing mail filters, visit settings.