Review at https://gerrit.osmocom.org/3537
osmux: Re-write osmux_snprintf After last small changes, 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 issue. 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/1 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: newchange Gerrit-Change-Id: I695771d099833842db37a415b636035d17f1bba7 Gerrit-PatchSet: 1 Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Owner: Pau Espin Pedrol <pes...@sysmocom.de>