diff mbox

[4/9] Add support for AMR frames to MNCC/RTP interface

Message ID 5350F977.6060201@eversberg.eu
State Changes Requested
Headers show

Commit Message

Andreas Eversberg April 18, 2014, 10:07 a.m. UTC 
Holger Hans Peter Freyther wrote:
> On Thu, Mar 20, 2014 at 10:46:03PM +0100, Holger Hans Peter Freyther wrote:
>
> dear andreas,
>
> do you intend to finish your patches?
>
> happy easter
> 	holger
dear holger,

i had that patch done already. (see attachment)

happy easter

andreas

Comments

Holger Freyther April 20, 2014, 2:30 p.m. UTC | #1 
On Fri, Apr 18, 2014 at 12:07:51PM +0200, Andreas Eversberg wrote:

hi,


> i had that patch done already. (see attachment)

what was the message id?  I didn't see it.

> happy easter

happy easter to you too.

> +	if (rtph->payload_type == RTP_PT_AMR) {
> +		new_msg = msgb_alloc(sizeof(struct gsm_data_frame) + 1
> +				     + payload_len, "GSM-DATA");
> +	} else {
> +		new_msg = msgb_alloc(sizeof(struct gsm_data_frame)
> +				     + payload_len, "GSM-DATA");
> +	}

I think the coding style asks us to ommit the {} here. Maybe use
different strings too in case we search for a memory leak?


> @@ -305,7 +324,13 @@ int rtp_send_frame(struct rtp_socket *rs, struct gsm_data_frame *frame)
>  	rtph->timestamp = htonl(rs->transmit.timestamp);
>  	rs->transmit.timestamp += duration;
>  	rtph->ssrc = htonl(rs->transmit.ssrc);
> -	memcpy(msg->data + sizeof(struct rtp_hdr), frame->data, payload_len);
> +	if (frame->msg_type == GSM_TCH_FRAME_AMR) {
> +		memcpy(msg->data + sizeof(struct rtp_hdr), frame->data + 1,
> +			payload_len);
> +	} else {
> +		memcpy(msg->data + sizeof(struct rtp_hdr), frame->data,
> +			payload_len);
> +	}


This lacks input validation. The code needs to check that the data
we read is within the bounds of the msgb and the data we write is within
the bounds too.
Holger Freyther April 29, 2014, 6:52 a.m. UTC | #2 
On Sun, Apr 20, 2014 at 04:30:20PM +0200, Holger Hans Peter Freyther wrote:

ping?

> > i had that patch done already. (see attachment)
> 
> what was the message id?  I didn't see it.

Could you please answer this one?

> This lacks input validation. The code needs to check that the data
> we read is within the bounds of the msgb and the data we write is within
> the bounds too.

Do you understand the severity? It is this kind of issue that OpenSSL
had with hearbleed. In this case our length is only a uint8_t and our
msgb is most likely over-allocated so we might be lucky that nothing
else will be leaked from the application.

holger
diff mbox

Patch

From 0c892b5bfb9343b735c96b93f153c95f145718eb Mon Sep 17 00:00:00 2001
From: Andreas Eversberg <jolly@eversberg.eu>
Date: Thu, 8 Mar 2012 14:39:19 +0100
Subject: [PATCH] Add support for AMR frames to MNCC/RTP interface

AMR rate is currently fixed to 5.9k.
---
 openbsc/src/libmsc/gsm_04_08.c  |  1 +
 openbsc/src/libtrau/rtp_proxy.c | 35 ++++++++++++++++++++++++++++++-----
 2 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/openbsc/src/libmsc/gsm_04_08.c b/openbsc/src/libmsc/gsm_04_08.c
index df93433..a0650ab 100644
--- a/openbsc/src/libmsc/gsm_04_08.c
+++ b/openbsc/src/libmsc/gsm_04_08.c
@@ -2953,6 +2953,7 @@  int mncc_tx_to_cc(struct gsm_network *net, int msg_type, void *arg)
 	case GSM_TCHF_FRAME:
 	case GSM_TCHF_FRAME_EFR:
 	case GSM_TCHH_FRAME:
+	case GSM_TCH_FRAME_AMR:
 		/* Find callref */
 		trans = trans_find_by_callref(net, data->callref);
 		if (!trans) {
diff --git a/openbsc/src/libtrau/rtp_proxy.c b/openbsc/src/libtrau/rtp_proxy.c
index 143bfa0..913d64d 100644
--- a/openbsc/src/libtrau/rtp_proxy.c
+++ b/openbsc/src/libtrau/rtp_proxy.c
@@ -192,21 +192,35 @@  static int rtp_decode(struct msgb *msg, uint32_t callref, struct msgb **data)
 			return -EINVAL;
 		}
 		break;
+	case RTP_PT_AMR:
+		break;
 	default:
 		DEBUGPC(DLMUX, "received RTP frame with unknown payload "
 			"type %d\n", rtph->payload_type);
 		return -EINVAL;
 	}
 
-	new_msg = msgb_alloc(sizeof(struct gsm_data_frame) + payload_len,
-				"GSM-DATA");
+	if (rtph->payload_type == RTP_PT_AMR) {
+		new_msg = msgb_alloc(sizeof(struct gsm_data_frame) + 1
+				     + payload_len, "GSM-DATA");
+	} else {
+		new_msg = msgb_alloc(sizeof(struct gsm_data_frame)
+				     + payload_len, "GSM-DATA");
+	}
 	if (!new_msg)
 		return -ENOMEM;
 	frame = (struct gsm_data_frame *)(new_msg->data);
 	frame->msg_type = msg_type;
 	frame->callref = callref;
-	memcpy(frame->data, payload, payload_len);
-	msgb_put(new_msg, sizeof(struct gsm_data_frame) + payload_len);
+	if (rtph->payload_type == RTP_PT_AMR) {
+		frame->data[0] = payload_len;
+		msgb_put(new_msg, sizeof(struct gsm_data_frame) + 1
+					 + payload_len);
+		memcpy(frame->data + 1, payload, payload_len);
+	} else {
+		msgb_put(new_msg, sizeof(struct gsm_data_frame) + payload_len);
+		memcpy(frame->data, payload, payload_len);
+	}
 
 	*data = new_msg;
 	return 0;
@@ -264,6 +278,11 @@  int rtp_send_frame(struct rtp_socket *rs, struct gsm_data_frame *frame)
 		payload_len = RTP_LEN_GSM_HALF;
 		duration = RTP_GSM_DURATION;
 		break;
+	case GSM_TCH_FRAME_AMR:
+		payload_type = RTP_PT_AMR;
+		payload_len = frame->data[0];
+		duration = RTP_GSM_DURATION;
+		break;
 	default:
 		DEBUGPC(DLMUX, "unsupported message type %d\n",
 			frame->msg_type);
@@ -305,7 +324,13 @@  int rtp_send_frame(struct rtp_socket *rs, struct gsm_data_frame *frame)
 	rtph->timestamp = htonl(rs->transmit.timestamp);
 	rs->transmit.timestamp += duration;
 	rtph->ssrc = htonl(rs->transmit.ssrc);
-	memcpy(msg->data + sizeof(struct rtp_hdr), frame->data, payload_len);
+	if (frame->msg_type == GSM_TCH_FRAME_AMR) {
+		memcpy(msg->data + sizeof(struct rtp_hdr), frame->data + 1,
+			payload_len);
+	} else {
+		memcpy(msg->data + sizeof(struct rtp_hdr), frame->data,
+			payload_len);
+	}
 	msgb_put(msg, sizeof(struct rtp_hdr) + payload_len);
 	msgb_enqueue(&rss->tx_queue, msg);
 	rss->bfd.when |= BSC_FD_WRITE;
-- 
1.8.3.2