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.orgHello 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>