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 <pes...@sysmocom.de>
Gerrit-Reviewer: Harald Welte <lafo...@gnumonks.org>
Gerrit-Reviewer: Holger Freyther <hol...@freyther.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Pablo Neira Ayuso <pa...@gnumonks.org>
Gerrit-Reviewer: Pau Espin Pedrol <pes...@sysmocom.de>

Reply via email to