bgpd: crash if attributes alone consume > 4096 bytes
This patch fixes a crash if attributes on a patch consume
more than 4096 bytes.
Signed-off-by: Daniel Walton <dwalton@cumulusnetworks.com>
diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c
index 0c47ab5..f42e544 100644
--- a/bgpd/bgp_packet.c
+++ b/bgpd/bgp_packet.c
@@ -153,6 +153,8 @@
struct bgp_info *binfo = NULL;
bgp_size_t total_attr_len = 0;
unsigned long attrlen_pos = 0;
+ int space_remaining = 0;
+ int space_needed = 0;
size_t mpattrlen_pos = 0;
size_t mpattr_pos = 0;
@@ -171,9 +173,12 @@
if (adv->binfo)
binfo = adv->binfo;
+ space_remaining = STREAM_CONCAT_REMAIN (s, snlri, STREAM_SIZE(s)) -
+ BGP_MAX_PACKET_SIZE_OVERFLOW;
+ space_needed = BGP_NLRI_LENGTH + bgp_packet_mpattr_prefix_size (afi, safi, &rn->p);
+
/* When remaining space can't include NLRI and it's length. */
- if (STREAM_CONCAT_REMAIN (s, snlri, STREAM_SIZE(s)) <=
- (BGP_NLRI_LENGTH + bgp_packet_mpattr_prefix_size(afi,safi,&rn->p)))
+ if (space_remaining < space_needed)
break;
/* If packet is empty, set attribute. */
@@ -217,6 +222,22 @@
&rn->p : NULL),
afi, safi,
from, prd, tag);
+ space_remaining = STREAM_CONCAT_REMAIN (s, snlri, STREAM_SIZE(s)) -
+ BGP_MAX_PACKET_SIZE_OVERFLOW;
+ space_needed = BGP_NLRI_LENGTH + bgp_packet_mpattr_prefix_size (afi, safi, &rn->p);;
+
+ /* If the attributes alone do not leave any room for NLRI then
+ * return */
+ if (space_remaining < space_needed)
+ {
+ zlog_err ("%s cannot send UPDATE, the attributes do not leave "
+ "room for NLRI", peer->host);
+ /* Flush the FIFO update queue */
+ while (adv)
+ adv = bgp_advertise_clean (peer, adv->adj, afi, safi);
+ return NULL;
+ }
+
}
if (afi == AFI_IP && safi == SAFI_UNICAST)
@@ -347,6 +368,8 @@
size_t attrlen_pos = 0;
size_t mplen_pos = 0;
u_char first_time = 1;
+ int space_remaining = 0;
+ int space_needed = 0;
s = peer->work;
stream_reset (s);
@@ -357,8 +380,12 @@
adj = adv->adj;
rn = adv->rn;
- if (STREAM_REMAIN (s)
- < (BGP_NLRI_LENGTH + BGP_TOTAL_ATTR_LEN + PSIZE (rn->p.prefixlen)))
+ space_remaining = STREAM_REMAIN (s) -
+ BGP_MAX_PACKET_SIZE_OVERFLOW;
+ space_needed = (BGP_NLRI_LENGTH + BGP_TOTAL_ATTR_LEN +
+ bgp_packet_mpattr_prefix_size (afi, safi, &rn->p));
+
+ if (space_remaining < space_needed)
break;
if (stream_empty (s))
diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c
index 669782d..010e224 100644
--- a/bgpd/bgpd.c
+++ b/bgpd/bgpd.c
@@ -868,7 +868,19 @@
/* Create buffers. */
peer->ibuf = stream_new (BGP_MAX_PACKET_SIZE);
peer->obuf = stream_fifo_new ();
- peer->work = stream_new (BGP_MAX_PACKET_SIZE);
+
+ /* We use a larger buffer for peer->work in the event that:
+ * - We RX a BGP_UPDATE where the attributes alone are just
+ * under BGP_MAX_PACKET_SIZE
+ * - The user configures an outbound route-map that does many as-path
+ * prepends or adds many communities. At most they can have CMD_ARGC_MAX
+ * args in a route-map so there is a finite limit on how large they can
+ * make the attributes.
+ *
+ * Having a buffer with BGP_MAX_PACKET_SIZE_OVERFLOW allows us to avoid bounds
+ * checking for every single attribute as we construct an UPDATE.
+ */
+ peer->work = stream_new (BGP_MAX_PACKET_SIZE + BGP_MAX_PACKET_SIZE_OVERFLOW);
peer->scratch = stream_new (BGP_MAX_PACKET_SIZE);
bgp_sync_init (peer);
diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h
index 3f6161c..40da02e 100644
--- a/bgpd/bgpd.h
+++ b/bgpd/bgpd.h
@@ -278,6 +278,8 @@
BGP_PEER_CONFED,
} bgp_peer_sort_t;
+#define BGP_MAX_PACKET_SIZE_OVERFLOW 1024
+
/* BGP neighbor structure. */
struct peer
{