From 68dc0c9bbda7c6075c53e646da949a9dc3ca3bc4 Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Thu, 21 Nov 2019 19:01:55 -0500 Subject: [PATCH] bgpd: use safe functions to work with ecom attrs Tons of insane just-so pointer math here where it is not needed. This is too smart. Use safer methods. Signed-off-by: Quentin Young --- bgpd/bgp_ecommunity.c | 219 ++++++++++++++++++++---------------------- bgpd/bgp_ecommunity.h | 2 - 2 files changed, 103 insertions(+), 118 deletions(-) diff --git a/bgpd/bgp_ecommunity.c b/bgpd/bgp_ecommunity.c index 942f0ae415c8..4c55a0764d2e 100644 --- a/bgpd/bgp_ecommunity.c +++ b/bgpd/bgp_ecommunity.c @@ -77,37 +77,36 @@ static void ecommunity_hash_free(struct ecommunity *ecom) else return 0. */ int ecommunity_add_val(struct ecommunity *ecom, struct ecommunity_val *eval) { - uint8_t *p; - int ret; int c; - /* When this is fist value, just add it. */ + /* When this is fist value, just add it. */ if (ecom->val == NULL) { - ecom->size++; - ecom->val = XMALLOC(MTYPE_ECOMMUNITY_VAL, ecom_length(ecom)); + ecom->size = 1; + ecom->val = XCALLOC(MTYPE_ECOMMUNITY_VAL, ECOMMUNITY_SIZE); memcpy(ecom->val, eval->val, ECOMMUNITY_SIZE); return 1; } /* If the value already exists in the structure return 0. */ c = 0; - for (p = ecom->val; c < ecom->size; p += ECOMMUNITY_SIZE, c++) { - ret = memcmp(p, eval->val, ECOMMUNITY_SIZE); + for (uint8_t *p = ecom->val; c < ecom->size; + p += ECOMMUNITY_SIZE, c++) { + int ret = memcmp(p, eval->val, ECOMMUNITY_SIZE); if (ret == 0) return 0; - if (ret > 0) + else if (ret > 0) break; } /* Add the value to the structure with numerical sorting. */ ecom->size++; - ecom->val = - XREALLOC(MTYPE_ECOMMUNITY_VAL, ecom->val, ecom_length(ecom)); + ecom->val = XREALLOC(MTYPE_ECOMMUNITY_VAL, ecom->val, + ecom->size * ECOMMUNITY_SIZE); - memmove(ecom->val + (c + 1) * ECOMMUNITY_SIZE, - ecom->val + c * ECOMMUNITY_SIZE, + memmove(ecom->val + ((c + 1) * ECOMMUNITY_SIZE), + ecom->val + (c * ECOMMUNITY_SIZE), (ecom->size - 1 - c) * ECOMMUNITY_SIZE); - memcpy(ecom->val + c * ECOMMUNITY_SIZE, eval->val, ECOMMUNITY_SIZE); + memcpy(ecom->val + (c * ECOMMUNITY_SIZE), eval->val, ECOMMUNITY_SIZE); return 1; } @@ -556,8 +555,8 @@ struct ecommunity *ecommunity_str2com(const char *str, int type, return ecom; } -static int ecommunity_rt_soo_str(char *buf, uint8_t *pnt, int type, - int sub_type, int format) +static int ecommunity_rt_soo_str(char *buf, size_t bufsz, uint8_t *pnt, + int type, int sub_type, int format) { int len = 0; const char *prefix; @@ -589,23 +588,25 @@ static int ecommunity_rt_soo_str(char *buf, uint8_t *pnt, int type, eas.val = (*pnt++ << 8); eas.val |= (*pnt++); - len = sprintf(buf, "%s%u:%u", prefix, eas.as, eas.val); + len = snprintf(buf, bufsz, "%s%u:%u", prefix, eas.as, eas.val); } else if (type == ECOMMUNITY_ENCODE_AS) { eas.as = (*pnt++ << 8); eas.as |= (*pnt++); pnt = ptr_get_be32(pnt, &eas.val); - len = sprintf(buf, "%s%u:%u", prefix, eas.as, eas.val); + len = snprintf(buf, bufsz, "%s%u:%u", prefix, eas.as, eas.val); } else if (type == ECOMMUNITY_ENCODE_IP) { memcpy(&eip.ip, pnt, 4); pnt += 4; eip.val = (*pnt++ << 8); eip.val |= (*pnt++); - len = sprintf(buf, "%s%s:%u", prefix, inet_ntoa(eip.ip), - eip.val); + len = snprintf(buf, bufsz, "%s%s:%u", prefix, inet_ntoa(eip.ip), + eip.val); } - (void)pnt; /* consume value */ + + /* consume value */ + (void)pnt; return len; } @@ -640,44 +641,31 @@ char *ecommunity_ecom2str(struct ecommunity *ecom, int format, int filter) uint8_t *pnt; uint8_t type = 0; uint8_t sub_type = 0; -#define ECOMMUNITY_STR_DEFAULT_LEN 64 +#define ECOMMUNITY_STRLEN 64 int str_size; - int str_pnt; char *str_buf; - int len = 0; - int first = 1; - if (ecom->size == 0) { - str_buf = XMALLOC(MTYPE_ECOMMUNITY_STR, 1); - str_buf[0] = '\0'; - return str_buf; - } + if (ecom->size == 0) + return XCALLOC(MTYPE_ECOMMUNITY_STR, 1); - /* Prepare buffer. */ - str_buf = XMALLOC(MTYPE_ECOMMUNITY_STR, ECOMMUNITY_STR_DEFAULT_LEN + 1); - str_size = ECOMMUNITY_STR_DEFAULT_LEN + 1; - str_buf[0] = '\0'; - str_pnt = 0; + /* ecom strlen + space + null term */ + str_size = (ecom->size * (ECOMMUNITY_STRLEN + 1)) + 1; + str_buf = XCALLOC(MTYPE_ECOMMUNITY_STR, str_size); + + char encbuf[128]; for (i = 0; i < ecom->size; i++) { int unk_ecom = 0; - - /* Make it sure size is enough. */ - while (str_pnt + ECOMMUNITY_STR_DEFAULT_LEN >= str_size) { - str_size *= 2; - str_buf = XREALLOC(MTYPE_ECOMMUNITY_STR, str_buf, - str_size); - } + memset(encbuf, 0x00, sizeof(encbuf)); /* Space between each value. */ - if (!first) { - str_buf[str_pnt++] = ' '; - len++; - } + if (i > 0) + strlcat(str_buf, " ", str_size); + /* Retrieve value field */ pnt = ecom->val + (i * 8); - /* High-order octet of type. */ + /* High-order octet is the type */ type = *pnt++; if (type == ECOMMUNITY_ENCODE_AS || type == ECOMMUNITY_ENCODE_IP @@ -696,15 +684,15 @@ char *ecommunity_ecom2str(struct ecommunity *ecom, int format, int filter) inet_ntop(AF_INET, ipv4, ipv4str, INET_ADDRSTRLEN); - len = sprintf(str_buf + str_pnt, - "NH:%s:%d", - ipv4str, pnt[5]); + snprintf(encbuf, sizeof(encbuf), + "NH:%s:%d", ipv4str, pnt[5]); } else unk_ecom = 1; - } else - len = ecommunity_rt_soo_str(str_buf + str_pnt, - pnt, type, sub_type, - format); + } else { + ecommunity_rt_soo_str(encbuf, sizeof(encbuf), + pnt, type, sub_type, + format); + } } else if (type == ECOMMUNITY_ENCODE_OPAQUE) { if (filter == ECOMMUNITY_ROUTE_TARGET) continue; @@ -712,13 +700,15 @@ char *ecommunity_ecom2str(struct ecommunity *ecom, int format, int filter) uint16_t tunneltype; memcpy(&tunneltype, pnt + 5, 2); tunneltype = ntohs(tunneltype); - len = sprintf(str_buf + str_pnt, "ET:%d", - tunneltype); + + snprintf(encbuf, sizeof(encbuf), "ET:%d", + tunneltype); } else if (*pnt == ECOMMUNITY_EVPN_SUBTYPE_DEF_GW) { - len = sprintf(str_buf + str_pnt, - "Default Gateway"); - } else + strlcpy(encbuf, "Default Gateway", + sizeof(encbuf)); + } else { unk_ecom = 1; + } } else if (type == ECOMMUNITY_ENCODE_EVPN) { if (filter == ECOMMUNITY_ROUTE_TARGET) continue; @@ -726,15 +716,15 @@ char *ecommunity_ecom2str(struct ecommunity *ecom, int format, int filter) struct ethaddr rmac; pnt++; memcpy(&rmac, pnt, ETH_ALEN); - len = sprintf( - str_buf + str_pnt, - "Rmac:%02x:%02x:%02x:%02x:%02x:%02x", - (uint8_t)rmac.octet[0], - (uint8_t)rmac.octet[1], - (uint8_t)rmac.octet[2], - (uint8_t)rmac.octet[3], - (uint8_t)rmac.octet[4], - (uint8_t)rmac.octet[5]); + + snprintf(encbuf, sizeof(encbuf), + "Rmac:%02x:%02x:%02x:%02x:%02x:%02x", + (uint8_t)rmac.octet[0], + (uint8_t)rmac.octet[1], + (uint8_t)rmac.octet[2], + (uint8_t)rmac.octet[3], + (uint8_t)rmac.octet[4], + (uint8_t)rmac.octet[5]); } else if (*pnt == ECOMMUNITY_EVPN_SUBTYPE_MACMOBILITY) { uint32_t seqnum; @@ -742,29 +732,30 @@ char *ecommunity_ecom2str(struct ecommunity *ecom, int format, int filter) memcpy(&seqnum, pnt + 2, 4); seqnum = ntohl(seqnum); - if (flags - & ECOMMUNITY_EVPN_SUBTYPE_MACMOBILITY_FLAG_STICKY) - len = sprintf(str_buf + str_pnt, - "MM:%u, sticky MAC", - seqnum); - else - len = sprintf(str_buf + str_pnt, - "MM:%u", seqnum); + + snprintf(encbuf, sizeof(encbuf), "MM:%u", + seqnum); + + if (CHECK_FLAG( + flags, + ECOMMUNITY_EVPN_SUBTYPE_MACMOBILITY_FLAG_STICKY)) + strlcat(encbuf, ", sticky MAC", + sizeof(encbuf)); } else if (*pnt == ECOMMUNITY_EVPN_SUBTYPE_ND) { uint8_t flags = *++pnt; - if (flags - & ECOMMUNITY_EVPN_SUBTYPE_ND_ROUTER_FLAG) - len = sprintf(str_buf + str_pnt, - "ND:Router Flag"); + if (CHECK_FLAG( + flags, + ECOMMUNITY_EVPN_SUBTYPE_ND_ROUTER_FLAG)) + strlcpy(encbuf, "ND:Router Flag", + sizeof(encbuf)); } else unk_ecom = 1; } else if (type == ECOMMUNITY_ENCODE_REDIRECT_IP_NH) { sub_type = *pnt++; if (sub_type == ECOMMUNITY_REDIRECT_IP_NH) { - len = sprintf( - str_buf + str_pnt, - "FS:redirect IP 0x%x", *(pnt+5)); + snprintf(encbuf, sizeof(encbuf), + "FS:redirect IP 0x%x", *(pnt + 5)); } else unk_ecom = 1; } else if (type == ECOMMUNITY_ENCODE_TRANS_EXP || @@ -772,38 +763,35 @@ char *ecommunity_ecom2str(struct ecommunity *ecom, int format, int filter) type == ECOMMUNITY_EXTENDED_COMMUNITY_PART_3) { sub_type = *pnt++; if (sub_type == ECOMMUNITY_REDIRECT_VRF) { - char buf[16]; - - memset(buf, 0, sizeof(buf)); - ecommunity_rt_soo_str(buf, (uint8_t *)pnt, - type & - ~ECOMMUNITY_ENCODE_TRANS_EXP, - ECOMMUNITY_ROUTE_TARGET, - ECOMMUNITY_FORMAT_DISPLAY); - len = snprintf(str_buf + str_pnt, - str_size - len, - "FS:redirect VRF %s", buf); + char buf[16] = {}; + ecommunity_rt_soo_str( + buf, sizeof(buf), (uint8_t *)pnt, + type & ~ECOMMUNITY_ENCODE_TRANS_EXP, + ECOMMUNITY_ROUTE_TARGET, + ECOMMUNITY_FORMAT_DISPLAY); + snprintf(encbuf, sizeof(encbuf), + "FS:redirect VRF %s", buf); } else if (type != ECOMMUNITY_ENCODE_TRANS_EXP) unk_ecom = 1; else if (sub_type == ECOMMUNITY_TRAFFIC_ACTION) { char action[64]; - char *ptr = action; if (*(pnt+3) == 1 << FLOWSPEC_TRAFFIC_ACTION_TERMINAL) - ptr += snprintf(ptr, sizeof(action), - "terminate (apply)"); + strlcpy(action, "terminate (apply)", + sizeof(action)); else - ptr += snprintf(ptr, sizeof(action), - "eval stops"); + strlcpy(action, "eval stops", + sizeof(action)); + if (*(pnt+3) == 1 << FLOWSPEC_TRAFFIC_ACTION_SAMPLE) - snprintf(ptr, sizeof(action) - - (size_t)(ptr-action), - ", sample"); - len = snprintf(str_buf + str_pnt, - str_size - len, - "FS:action %s", action); + strlcat(action, ", sample", + sizeof(action)); + + + snprintf(encbuf, sizeof(encbuf), "FS:action %s", + action); } else if (sub_type == ECOMMUNITY_TRAFFIC_RATE) { union traffic_rate data; @@ -811,20 +799,19 @@ char *ecommunity_ecom2str(struct ecommunity *ecom, int format, int filter) data.rate_byte[2] = *(pnt+3); data.rate_byte[1] = *(pnt+4); data.rate_byte[0] = *(pnt+5); - len = sprintf( - str_buf + str_pnt, - "FS:rate %f", data.rate_float); + snprintf(encbuf, sizeof(encbuf), "FS:rate %f", + data.rate_float); } else if (sub_type == ECOMMUNITY_TRAFFIC_MARKING) { - len = sprintf( - str_buf + str_pnt, - "FS:marking %u", *(pnt+5)); + snprintf(encbuf, sizeof(encbuf), + "FS:marking %u", *(pnt + 5)); } else if (*pnt == ECOMMUNITY_EVPN_SUBTYPE_ES_IMPORT_RT) { struct ethaddr mac; memcpy(&mac, pnt, ETH_ALEN); - len = sprintf( - str_buf + str_pnt, + + snprintf( + encbuf, sizeof(encbuf), "ES-Import-Rt:%02x:%02x:%02x:%02x:%02x:%02x", (uint8_t)mac.octet[0], (uint8_t)mac.octet[1], @@ -840,11 +827,11 @@ char *ecommunity_ecom2str(struct ecommunity *ecom, int format, int filter) } if (unk_ecom) - len = sprintf(str_buf + str_pnt, "UNK:%d, %d", - type, sub_type); + snprintf(encbuf, sizeof(encbuf), "UNK:%d, %d", type, + sub_type); - str_pnt += len; - first = 0; + int r = strlcat(str_buf, encbuf, str_size); + assert(r < str_size); } return str_buf; diff --git a/bgpd/bgp_ecommunity.h b/bgpd/bgp_ecommunity.h index 249e5bf7de41..ae64f41ca155 100644 --- a/bgpd/bgp_ecommunity.h +++ b/bgpd/bgp_ecommunity.h @@ -105,8 +105,6 @@ struct ecommunity_val { char val[ECOMMUNITY_SIZE]; }; -#define ecom_length(X) ((X)->size * ECOMMUNITY_SIZE) - /* * Encode BGP Route Target AS:nn. */