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 more 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

5 files changed, 170 insertions, 72 deletions

diff --git a/include/osmo-bts/Makefile.am b/include/osmo-bts/Makefile.am
index f24da539..0e45d708 100644
--- a/include/osmo-bts/Makefile.am
+++ b/include/osmo-bts/Makefile.am
@@ -12,6 +12,7 @@ noinst_HEADERS = \
 	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 00000000..822ee4a4
--- /dev/null
+++ b/include/osmo-bts/rtp_input_preen.h
@@ -0,0 +1,20 @@
+/*
+ * RTP input validation function: 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,
+	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 830f940c..32f644c3 100644
--- a/src/common/Makefile.am
+++ b/src/common/Makefile.am
@@ -36,6 +36,7 @@ libbts_a_SOURCES = \
 	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 f24bc2f7..f1692c68 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 @@ static int l1sap_ph_rts_ind(struct gsm_bts_trx *trx,
 	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 @@ static int l1sap_tch_rts_ind(struct gsm_bts_trx *trx,
 	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 @@ void l1sap_rtp_rx_cb(struct osmo_rtp_socket *rs, const uint8_t *rtp_pl,
 	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:
+		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/rtp_input_preen.c b/src/common/rtp_input_preen.c
new file mode 100644
index 00000000..90ff6da8
--- /dev/null
+++ b/src/common/rtp_input_preen.c
@@ -0,0 +1,121 @@
+/*
+ * This module implements a helper function for the RTP input path:
+ * 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 amr_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;
+}
+
+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;
+}
+
+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;	/* 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 (!amr_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;
+	default:
+		return PL_DECISION_ACCEPT;
+	}
+}