From 0a91ff55b89a5bc163cf0690125a7a142d5ff94f Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Tue, 19 May 2015 17:58:10 -0700 Subject: [PATCH] BGP crashes if attributes alone consume > 4096 bytes --- bgpd/bgp_packet.c | 35 +++++++++++++++++++++++++++++++---- bgpd/bgpd.c | 14 +++++++++++++- bgpd/bgpd.h | 1 + 3 files changed, 45 insertions(+), 5 deletions(-) diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index ec7b0d60b7..524c756113 100644 --- a/bgpd/bgp_packet.c +++ b/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)) <= - (BGP_NLRI_LENGTH + PSIZE (rn->p.prefixlen))) + if (space_remaining < space_needed) 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) - < (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 + + PSIZE (rn->p.prefixlen)); + + if (space_remaining < space_needed) break; if (stream_empty (s)) diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 653630e29a..b205df262d 100644 --- a/bgpd/bgpd.c +++ b/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); diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h index 3d4e03e5f1..2e0419d303 100644 --- a/bgpd/bgpd.h +++ b/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 -- 2.39.5