[PATCH] libosmo-netif[master]: osmux: Re-write osmux_snprintf

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

Pau Espin Pedrol gerrit-no-reply at lists.osmocom.org
Fri Sep 1 08:32:32 UTC 2017


Hello Harald Welte, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

    https://gerrit.osmocom.org/3537

to look at the new patch set (#2).

osmux: Re-write osmux_snprintf

After last buffer overflow fix to osmux_snprintf in
7cca0da1cc58bd589989684147ae3a0cd5819902, it was spotted that some cases may
still be able to make osmux_snprintf acces unexpected memory. This patch attemps
to try harder at fixing those issues.

See OS#2443 for more information.

Change-Id: I695771d099833842db37a415b636035d17f1bba7
---
M src/osmux.c
1 file changed, 40 insertions(+), 50 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/libosmo-netif refs/changes/37/3537/2

diff --git a/src/osmux.c b/src/osmux.c
index b3c43e2..2b1bef8 100644
--- a/src/osmux.c
+++ b/src/osmux.c
@@ -846,19 +846,17 @@
 	h->rtp_ssrc = rtp_ssrc;
 }
 
-#define SNPRINTF_BUFFER_SIZE(ret, size, len, offset)	\
-	size -= ret;					\
-	if (ret > len)					\
-		ret = len;				\
-	offset += ret;					\
-	len -= ret;
+#define SNPRINTF_BUFFER_SIZE(ret, buffer_offset, size)		\
+	if (ret < 0)						\
+		return ret;					\
+	if (ret >= size - buf_offset - 1)			\
+		return size - 1; /* full, early return */	\
+	buf_offset += ret;					\
+
 
 static int osmux_snprintf_header(char *buf, size_t size, struct osmux_hdr *osmuxh)
 {
-	int ret;
-	int len = size, offset = 0;
-
-	ret = snprintf(buf, len, "OSMUX seq=%03u ccid=%03u "
+	return snprintf(buf, size, "OSMUX seq=%03u ccid=%03u "
 				 "ft=%01u ctr=%01u "
 				 "amr_f=%01u amr_q=%01u "
 				 "amr_ft=%02u amr_cmr=%02u ",
@@ -866,82 +864,74 @@
 			osmuxh->ft, osmuxh->ctr,
 			osmuxh->amr_f, osmuxh->amr_q,
 			osmuxh->amr_ft, osmuxh->amr_cmr);
-	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
-
-	return offset;
 }
 
 static int osmux_snprintf_payload(char *buf, size_t size,
 				  const uint8_t *payload, int payload_len)
 {
+	unsigned int buf_offset = 0;
 	int ret, i;
-	int len = size, offset = 0;
 
-	ret = snprintf(buf+offset, len, "[ ");
-	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	ret = snprintf(buf + buf_offset, size - buf_offset, "[ ");
+	SNPRINTF_BUFFER_SIZE(ret, buf_offset, size);
 
-	for (i=0; i<payload_len; i++) {
-		ret = snprintf(buf+offset, len, "%02x ", payload[i]);
-		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	for (i = 0; i < payload_len; i++) {
+		ret = snprintf(buf + buf_offset, size - buf_offset, "%02x ", payload[i]);
+		SNPRINTF_BUFFER_SIZE(ret, buf_offset, size);
 	}
 
-	ret = snprintf(buf+offset, len, "] ");
-	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	ret = snprintf(buf + buf_offset, size - buf_offset, "] ");
+	SNPRINTF_BUFFER_SIZE(ret, buf_offset, size);
 
-	return offset;
+	return buf_offset;
 }
 
-
+/*! print osmux header fields and payload from msg into buffer buf.
+ *  \param[out] buf buffer to store the output into
+ *  \param[in] len length of buf in bytes
+ *  \param[in] msgb message buffer containing one or more osmux frames
+ *  \returns amount of bytes printed, in range (0,size-1), excluding the null byte at the end. Negative value on error.
+ */
 int osmux_snprintf(char *buf, size_t size, struct msgb *msg)
 {
 	int ret;
-	unsigned int offset = 0;
-	int msg_len = msg->len, len = size;
+	unsigned int msg_offset = 0;
+	unsigned int buf_offset = 0;
 	struct osmux_hdr *osmuxh;
-	int this_len, msg_off = 0;
 
-	while (msg_len > 0) {
-		if (msg_len < sizeof(struct osmux_hdr)) {
+	while (msg->len > msg_offset) {
+		if (msg->len - msg_offset < sizeof(struct osmux_hdr)) {
 			LOGP(DLMIB, LOGL_ERROR,
-			     "No room for OSMUX header: only %d bytes\n",
-			     msg_len);
+			     "No room for OSMUX header: only %u bytes\n",
+			     msg->len - msg_offset);
 			return -1;
 		}
-		osmuxh = (struct osmux_hdr *)((uint8_t *)msg->data + msg_off);
+		osmuxh = (struct osmux_hdr *)((uint8_t *)msg->data + msg_offset);
 
-		if (!osmo_amr_ft_valid(osmuxh->amr_ft)) {
-			LOGP(DLMIB, LOGL_ERROR, "Bad AMR FT %d, skipping\n",
+		if (osmuxh->ft == OSMUX_FT_VOICE_AMR && !osmo_amr_ft_valid(osmuxh->amr_ft)) {
+			LOGP(DLMIB, LOGL_ERROR, "Bad AMR FT 0x%x, skipping\n",
 			     osmuxh->amr_ft);
 			return -1;
 		}
 
-		ret = osmux_snprintf_header(buf+offset, size, osmuxh);
-		if (ret < 0)
-			break;
-		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+		ret = osmux_snprintf_header(buf + buf_offset, size - buf_offset, osmuxh);
+		SNPRINTF_BUFFER_SIZE(ret, buf_offset, size);
 
-		this_len = sizeof(struct osmux_hdr) +
-			   osmux_get_payload_len(osmuxh);
-		msg_off += this_len;
+		msg_offset += sizeof(struct osmux_hdr) + osmux_get_payload_len(osmuxh);
 
-		if (msg_len < this_len) {
+		if (msg_offset > msg->len) {
 			LOGP(DLMIB, LOGL_ERROR,
 			     "No room for OSMUX payload: only %d bytes\n",
-			     msg_len);
+			     msg->len - msg_offset);
 			return -1;
 		}
 
-		ret = osmux_snprintf_payload(buf+offset, size,
+		ret = osmux_snprintf_payload(buf + buf_offset, size - buf_offset,
 					     osmux_get_payload(osmuxh),
 					     osmux_get_payload_len(osmuxh));
-		if (ret < 0)
-			break;
-		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
-
-		msg_len -= this_len;
+		SNPRINTF_BUFFER_SIZE(ret, buf_offset, size);
 	}
-
-	return offset;
+	return buf_offset;
 }
 
 /*! @} */

-- 
To view, visit https://gerrit.osmocom.org/3537
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I695771d099833842db37a415b636035d17f1bba7
Gerrit-PatchSet: 2
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Owner: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Holger Freyther <holger at freyther.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Pablo Neira Ayuso <pablo at gnumonks.org>
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>



More information about the gerrit-log mailing list