BGP crashes if attributes alone consume > 4096 bytes

bgpd/bgp_packet.c

@@ -153,6 +153,8 @@ bgp_update_packet (struct peer *peer, afi_t afi, safi_t safi)
   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 @@ bgp_update_packet (struct peer *peer, afi_t afi, safi_t safi)
       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 + PSIZE (rn->p.prefixlen);
+
       /* When remaining space can't include NLRI and it's length.  */
-      if (STREAM_CONCAT_REMAIN (s, snlri, STREAM_SIZE(s)) <=
+      if (space_remaining < space_needed)
-	  (BGP_NLRI_LENGTH + PSIZE (rn->p.prefixlen)))
 	break;
 
       /* If packet is empty, set attribute. */
@@ -207,6 +212,22 @@ bgp_update_packet (struct peer *peer, afi_t afi, safi_t safi)
 	                                         adv->baa->attr,
 	                                         NULL, afi, safi,
 	                                         from, NULL, NULL);
+
+          space_remaining = STREAM_CONCAT_REMAIN (s, snlri, STREAM_SIZE(s)) -
+                            BGP_MAX_PACKET_SIZE_OVERFLOW;
+          space_needed = BGP_NLRI_LENGTH + PSIZE (rn->p.prefixlen);
+
+          /* 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)
@@ -341,6 +362,8 @@ bgp_withdraw_packet (struct peer *peer, afi_t afi, safi_t safi)
   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);
@@ -351,8 +374,12 @@ bgp_withdraw_packet (struct peer *peer, afi_t afi, safi_t safi)
       adj = adv->adj;
       rn = adv->rn;
 
-      if (STREAM_REMAIN (s)
+      space_remaining = STREAM_REMAIN (s) -
-	  < (BGP_NLRI_LENGTH + BGP_TOTAL_ATTR_LEN + PSIZE (rn->p.prefixlen)))
+                        BGP_MAX_PACKET_SIZE_OVERFLOW;
+      space_needed = (BGP_NLRI_LENGTH + BGP_TOTAL_ATTR_LEN +
+                      PSIZE (rn->p.prefixlen));
+
+      if (space_remaining < space_needed)
 	break;
 
       if (stream_empty (s))

bgpd/bgpd.c

@@ -873,7 +873,19 @@ peer_new (struct bgp *bgp)
   /* 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);
 
 

bgpd/bgpd.h

@@ -329,6 +329,7 @@ typedef enum
 #define BGP_MARKER_SIZE		                16
 #define BGP_HEADER_SIZE		                19
 #define BGP_MAX_PACKET_SIZE                   4096
+#define BGP_MAX_PACKET_SIZE_OVERFLOW          1024
 
 /* BGP neighbor structure. */
 struct peer