From: David Lamparter <equinox@opensourcerouting.org>
Date: Thu, 7 Sep 2017 13:58:18 +0000 (+0200)
Subject: *: fix be32 reading / 24-bit left shift
X-Git-Tag: frr-4.0-dev~318^2
X-Git-Url: https://git.puffer.fish/?a=commitdiff_plain;h=937652c6e43fc74ba969bbace475bdf929cdc5d0;p=mirror%2Ffrr.git

*: fix be32 reading / 24-bit left shift

Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
---

diff --git a/bgpd/bgp_attr_evpn.c b/bgpd/bgp_attr_evpn.c
index 6ead059261..300c9ddb50 100644
--- a/bgpd/bgp_attr_evpn.c
+++ b/bgpd/bgp_attr_evpn.c
@@ -144,11 +144,8 @@ u_int32_t bgp_attr_mac_mobility_seqnum(struct attr *attr, u_char *sticky)
 			*sticky = 0;
 
 		pnt++;
-		seq_num = (*pnt++ << 24);
-		seq_num |= (*pnt++ << 16);
-		seq_num |= (*pnt++ << 8);
-		seq_num |= (*pnt++);
-
+		pnt = ptr_get_be32(pnt, &seq_num);
+		(void)pnt; /* consume value */
 		return seq_num;
 	}
 
diff --git a/bgpd/bgp_clist.c b/bgpd/bgp_clist.c
index b62a0a540c..f3bae9535c 100644
--- a/bgpd/bgp_clist.c
+++ b/bgpd/bgp_clist.c
@@ -25,6 +25,7 @@
 #include "memory.h"
 #include "queue.h"
 #include "filter.h"
+#include "stream.h"
 
 #include "bgpd/bgpd.h"
 #include "bgpd/bgp_community.h"
@@ -465,20 +466,10 @@ static char *lcommunity_str_get(struct lcommunity *lcom, int i)
 	str = pnt = XMALLOC(MTYPE_LCOMMUNITY_STR, 48);
 
 	ptr = (u_char *)lcomval.val;
-	globaladmin = (*ptr++ << 24);
-	globaladmin |= (*ptr++ << 16);
-	globaladmin |= (*ptr++ << 8);
-	globaladmin |= (*ptr++);
-
-	localdata1 = (*ptr++ << 24);
-	localdata1 |= (*ptr++ << 16);
-	localdata1 |= (*ptr++ << 8);
-	localdata1 |= (*ptr++);
-
-	localdata2 = (*ptr++ << 24);
-	localdata2 |= (*ptr++ << 16);
-	localdata2 |= (*ptr++ << 8);
-	localdata2 |= (*ptr++);
+	ptr = ptr_get_be32(ptr, &globaladmin);
+	ptr = ptr_get_be32(ptr, &localdata1);
+	ptr = ptr_get_be32(ptr, &localdata2);
+	(void)ptr; /* consume value */
 
 	sprintf(pnt, "%u:%u:%u", globaladmin, localdata1, localdata2);
 	pnt += strlen(pnt);
diff --git a/bgpd/bgp_ecommunity.c b/bgpd/bgp_ecommunity.c
index ec52422d4c..897b1c7508 100644
--- a/bgpd/bgp_ecommunity.c
+++ b/bgpd/bgp_ecommunity.c
@@ -27,6 +27,7 @@
 #include "queue.h"
 #include "filter.h"
 #include "jhash.h"
+#include "stream.h"
 
 #include "bgpd/bgpd.h"
 #include "bgpd/bgp_ecommunity.h"
@@ -583,10 +584,7 @@ static int ecommunity_rt_soo_str(char *buf, u_int8_t *pnt, int type,
 
 	/* Put string into buffer.  */
 	if (type == ECOMMUNITY_ENCODE_AS4) {
-		eas.as = (*pnt++ << 24);
-		eas.as |= (*pnt++ << 16);
-		eas.as |= (*pnt++ << 8);
-		eas.as |= (*pnt++);
+		pnt = ptr_get_be32(pnt, &eas.as);
 		eas.val = (*pnt++ << 8);
 		eas.val |= (*pnt++);
 
@@ -594,11 +592,7 @@ static int ecommunity_rt_soo_str(char *buf, u_int8_t *pnt, int type,
 	} else if (type == ECOMMUNITY_ENCODE_AS) {
 		eas.as = (*pnt++ << 8);
 		eas.as |= (*pnt++);
-
-		eas.val = (*pnt++ << 24);
-		eas.val |= (*pnt++ << 16);
-		eas.val |= (*pnt++ << 8);
-		eas.val |= (*pnt++);
+		pnt = ptr_get_be32(pnt, &eas.val);
 
 		len = sprintf(buf, "%s%u:%u", prefix, eas.as, eas.val);
 	} else if (type == ECOMMUNITY_ENCODE_IP) {
@@ -610,6 +604,7 @@ static int ecommunity_rt_soo_str(char *buf, u_int8_t *pnt, int type,
 		len = sprintf(buf, "%s%s:%u", prefix, inet_ntoa(eip.ip),
 			      eip.val);
 	}
+	(void)pnt; /* consume value */
 
 	return len;
 }
diff --git a/bgpd/bgp_encap_tlv.c b/bgpd/bgp_encap_tlv.c
index 5c0cc40f16..4457501613 100644
--- a/bgpd/bgp_encap_tlv.c
+++ b/bgpd/bgp_encap_tlv.c
@@ -22,6 +22,7 @@
 #include "memory.h"
 #include "prefix.h"
 #include "filter.h"
+#include "stream.h"
 
 #include "bgpd.h"
 #include "bgp_attr.h"
@@ -470,8 +471,7 @@ static int subtlv_decode_encap_l2tpv3_over_ip(
 		return -1;
 	}
 
-	st->sessionid = (subtlv->value[0] << 24) | (subtlv->value[1] << 16)
-			| (subtlv->value[2] << 8) | subtlv->value[3];
+	ptr_get_be32(subtlv->value, &st->sessionid);
 	st->cookie_length = subtlv->length - 4;
 	if (st->cookie_length > sizeof(st->cookie)) {
 		zlog_debug("%s, subtlv length %d is greater than %d", __func__,
@@ -491,8 +491,7 @@ static int subtlv_decode_encap_gre(struct bgp_attr_encap_subtlv *subtlv,
 			   subtlv->length);
 		return -1;
 	}
-	st->gre_key = (subtlv->value[0] << 24) | (subtlv->value[1] << 16)
-		      | (subtlv->value[2] << 8) | subtlv->value[3];
+	ptr_get_be32(subtlv->value, &st->gre_key);
 	return 0;
 }
 
@@ -545,8 +544,7 @@ static int subtlv_decode_color(struct bgp_attr_encap_subtlv *subtlv,
 			   __func__);
 		return -1;
 	}
-	st->color = (subtlv->value[4] << 24) | (subtlv->value[5] << 16)
-		    | (subtlv->value[6] << 8) | subtlv->value[7];
+	ptr_get_be32(subtlv->value + 4, &st->color);
 	return 0;
 }
 
@@ -580,16 +578,13 @@ subtlv_decode_remote_endpoint(struct bgp_attr_encap_subtlv *subtlv,
 	}
 	if (subtlv->length == 8) {
 		st->family = AF_INET;
-		st->ip_address.v4.s_addr =
-			((subtlv->value[0] << 24) | (subtlv->value[1] << 16)
-			 | (subtlv->value[2] << 8) | subtlv->value[3]);
+		memcpy(&st->ip_address.v4.s_addr, subtlv->value, 4);
 	} else {
 		st->family = AF_INET6;
 		memcpy(&(st->ip_address.v6.s6_addr), subtlv->value, 16);
 	}
 	i = subtlv->length - 4;
-	st->as4 = ((subtlv->value[i] << 24) | (subtlv->value[i + 1] << 16)
-		   | (subtlv->value[i + 2] << 8) | subtlv->value[i + 3]);
+	ptr_get_be32(subtlv->value + i, &st->as4);
 	return 0;
 }
 
diff --git a/bgpd/bgp_evpn_vty.c b/bgpd/bgp_evpn_vty.c
index 2410f30ddf..17511827b1 100644
--- a/bgpd/bgp_evpn_vty.c
+++ b/bgpd/bgp_evpn_vty.c
@@ -22,6 +22,7 @@
 #include "command.h"
 #include "prefix.h"
 #include "lib/json.h"
+#include "stream.h"
 
 #include "bgpd/bgpd.h"
 #include "bgpd/bgp_table.h"
@@ -87,11 +88,7 @@ static void display_import_rt(struct vty *vty, struct irt_node *irt,
 	case ECOMMUNITY_ENCODE_AS:
 		eas.as = (*pnt++ << 8);
 		eas.as |= (*pnt++);
-
-		eas.val = (*pnt++ << 24);
-		eas.val |= (*pnt++ << 16);
-		eas.val |= (*pnt++ << 8);
-		eas.val |= (*pnt++);
+		pnt = ptr_get_be32(pnt, &eas.val);
 
 		snprintf(rt_buf, RT_ADDRSTRLEN, "%u:%u", eas.as, eas.val);
 
@@ -119,11 +116,7 @@ static void display_import_rt(struct vty *vty, struct irt_node *irt,
 		break;
 
 	case ECOMMUNITY_ENCODE_AS4:
-		eas.as = (*pnt++ << 24);
-		eas.as |= (*pnt++ << 16);
-		eas.as |= (*pnt++ << 8);
-		eas.as |= (*pnt++);
-
+		pnt = ptr_get_be32(pnt, &eas.val);
 		eas.val = (*pnt++ << 8);
 		eas.val |= (*pnt++);
 
diff --git a/bgpd/bgp_lcommunity.c b/bgpd/bgp_lcommunity.c
index d75f33f58b..54e9fd8894 100644
--- a/bgpd/bgp_lcommunity.c
+++ b/bgpd/bgp_lcommunity.c
@@ -26,6 +26,7 @@
 #include "command.h"
 #include "filter.h"
 #include "jhash.h"
+#include "stream.h"
 
 #include "bgpd/bgpd.h"
 #include "bgpd/bgp_lcommunity.h"
@@ -431,20 +432,10 @@ char *lcommunity_lcom2str(struct lcommunity *lcom, int format)
 
 		pnt = lcom->val + (i * LCOMMUNITY_SIZE);
 
-		globaladmin = (*pnt++ << 24);
-		globaladmin |= (*pnt++ << 16);
-		globaladmin |= (*pnt++ << 8);
-		globaladmin |= (*pnt++);
-
-		localdata1 = (*pnt++ << 24);
-		localdata1 |= (*pnt++ << 16);
-		localdata1 |= (*pnt++ << 8);
-		localdata1 |= (*pnt++);
-
-		localdata2 = (*pnt++ << 24);
-		localdata2 |= (*pnt++ << 16);
-		localdata2 |= (*pnt++ << 8);
-		localdata2 |= (*pnt++);
+		pnt = ptr_get_be32(pnt, &globaladmin);
+		pnt = ptr_get_be32(pnt, &localdata1);
+		pnt = ptr_get_be32(pnt, &localdata2);
+		(void)pnt; /* consume value */
 
 		len = sprintf(str_buf + str_pnt, "%u:%u:%u", globaladmin,
 			      localdata1, localdata2);
diff --git a/bgpd/bgp_rd.c b/bgpd/bgp_rd.c
index a15828bd36..2b676e052b 100644
--- a/bgpd/bgp_rd.c
+++ b/bgpd/bgp_rd.c
@@ -63,21 +63,13 @@ void decode_rd_as(u_char *pnt, struct rd_as *rd_as)
 {
 	rd_as->as = (u_int16_t)*pnt++ << 8;
 	rd_as->as |= (u_int16_t)*pnt++;
-
-	rd_as->val = ((u_int32_t)*pnt++ << 24);
-	rd_as->val |= ((u_int32_t)*pnt++ << 16);
-	rd_as->val |= ((u_int32_t)*pnt++ << 8);
-	rd_as->val |= (u_int32_t)*pnt;
+	ptr_get_be32(pnt, &rd_as->val);
 }
 
 /* type == RD_TYPE_AS4 */
 void decode_rd_as4(u_char *pnt, struct rd_as *rd_as)
 {
-	rd_as->as = (u_int32_t)*pnt++ << 24;
-	rd_as->as |= (u_int32_t)*pnt++ << 16;
-	rd_as->as |= (u_int32_t)*pnt++ << 8;
-	rd_as->as |= (u_int32_t)*pnt++;
-
+	pnt = ptr_get_be32(pnt, &rd_as->as);
 	rd_as->val = ((u_int16_t)*pnt++ << 8);
 	rd_as->val |= (u_int16_t)*pnt;
 }
diff --git a/bgpd/rfapi/rfapi_import.c b/bgpd/rfapi/rfapi_import.c
index 3f427c3903..d963a759ac 100644
--- a/bgpd/rfapi/rfapi_import.c
+++ b/bgpd/rfapi/rfapi_import.c
@@ -33,6 +33,7 @@
 #include "lib/log.h"
 #include "lib/skiplist.h"
 #include "lib/thread.h"
+#include "lib/stream.h"
 
 #include "bgpd/bgpd.h"
 #include "bgpd/bgp_ecommunity.h"
@@ -1079,10 +1080,7 @@ int rfapiEcommunityGetEthernetTag(struct ecommunity *ecom, uint16_t *tag_id)
 
 			if (*p++ == ECOMMUNITY_ROUTE_TARGET) {
 				if (encode == ECOMMUNITY_ENCODE_AS4) {
-					as = (*p++ << 24);
-					as |= (*p++ << 16);
-					as |= (*p++ << 8);
-					as |= (*p++);
+					p = ptr_get_be32(p, &as);
 				} else if (encode == ECOMMUNITY_ENCODE_AS) {
 					as = (*p++ << 8);
 					as |= (*p++);
diff --git a/lib/skiplist.c b/lib/skiplist.c
index 7acc78f563..a546bb44c0 100644
--- a/lib/skiplist.c
+++ b/lib/skiplist.c
@@ -593,8 +593,8 @@ static void *scramble(int i)
 {
 	uintptr_t result;
 
-	result = (i & 0xff) << 24;
-	result |= (i >> 8);
+	result = (unsigned)(i & 0xff) << 24;
+	result |= (unsigned)i >> 8;
 
 	return (void *)result;
 }
diff --git a/lib/stream.c b/lib/stream.c
index 577fa257df..f88689f677 100644
--- a/lib/stream.c
+++ b/lib/stream.c
@@ -394,7 +394,7 @@ u_int32_t stream_getl_from(struct stream *s, size_t from)
 		return 0;
 	}
 
-	l = s->data[from++] << 24;
+	l = (unsigned)(s->data[from++]) << 24;
 	l |= s->data[from++] << 16;
 	l |= s->data[from++] << 8;
 	l |= s->data[from];
@@ -426,7 +426,7 @@ u_int32_t stream_getl(struct stream *s)
 		return 0;
 	}
 
-	l = s->data[s->getp++] << 24;
+	l = (unsigned)(s->data[s->getp++]) << 24;
 	l |= s->data[s->getp++] << 16;
 	l |= s->data[s->getp++] << 8;
 	l |= s->data[s->getp++];
diff --git a/lib/stream.h b/lib/stream.h
index 33dd64c4a3..7dcdca69f6 100644
--- a/lib/stream.h
+++ b/lib/stream.h
@@ -239,4 +239,22 @@ extern struct stream *stream_fifo_head(struct stream_fifo *fifo);
 extern void stream_fifo_clean(struct stream_fifo *fifo);
 extern void stream_fifo_free(struct stream_fifo *fifo);
 
+/* This is here because "<< 24" is particularly problematic in C.
+ * This is because the left operand of << is integer-promoted, which means
+ * an uint8_t gets converted into a *signed* int.  Shifting into the sign
+ * bit of a signed int is theoretically undefined behaviour, so - the left
+ * operand needs to be cast to unsigned.
+ *
+ * This is not a problem for 16- or 8-bit values (they don't reach the sign
+ * bit), for 64-bit values (you need to cast them anyway), and neither for
+ * encoding (because it's downcasted.)
+ */
+static inline uint8_t *ptr_get_be32(uint8_t *ptr, uint32_t *out)
+{
+	uint32_t tmp;
+	memcpy(&tmp, ptr, sizeof(tmp));
+	*out = ntohl(tmp);
+	return ptr + 4;
+}
+
 #endif /* _ZEBRA_STREAM_H */
diff --git a/ospfd/ospf_opaque.h b/ospfd/ospf_opaque.h
index 2470cd2e2b..9dc1f92f4d 100644
--- a/ospfd/ospf_opaque.h
+++ b/ospfd/ospf_opaque.h
@@ -46,7 +46,8 @@
 #define GET_OPAQUE_ID(lsid) ((u_int32_t)(lsid)&LSID_OPAQUE_ID_MASK)
 
 #define SET_OPAQUE_LSID(type, id)                                              \
-	((((type) << 24) & LSID_OPAQUE_TYPE_MASK) | ((id)&LSID_OPAQUE_ID_MASK))
+	((((unsigned)(type) << 24) & LSID_OPAQUE_TYPE_MASK)                    \
+	 | ((id) & LSID_OPAQUE_ID_MASK))
 
 /*
  * Opaque LSA types will be assigned by IANA.