From 556beacf106ac75043f63d4e10d5d3046f1d2163 Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Tue, 27 Apr 2021 16:20:27 -0400 Subject: [PATCH] bgpd: rework BGP_MAX_PACKET_SIZE & friends BGP_MAX_PACKET_SIZE no longer represented the absolute maximum BGP packet size as it did before, instead it was defined as 4096 bytes, which is the maximum unless extended message capability is negotiated, in which case the maximum goes to 65k. That introduced at least one bug - last_reset_cause was undersized for extended messages, and when sending an extended message > 4096 bytes back to a peer as part of NOTIFY data would trigger a bounds check assert. This patch redefines the macro to restore its previous meaning, introduces a new macro - BGP_STANDARD_MESSAGE_MAX_PACKET_SIZE - to represent the 4096 byte size, and renames the extended size to BGP_EXTENDED_MESSAGE_MAX_PACKET_SIZE for consistency. Code locations that definitely should use the small size have been updated, locations that semantically always need whatever the max is, no matter what that is, use BGP_MAX_PACKET_SIZE. BGP_EXTENDED_MESSAGE_MAX_PACKET_SIZE should only be used as a constant when storing what the negotiated max size is for use at runtime and to define BGP_MAX_PACKET_SIZE. Unless there is a future standard that introduces a third valid size it should not be used for any other purpose. Signed-off-by: Quentin Young --- bgpd/bgp_dump.c | 7 ++++--- bgpd/bgp_open.c | 6 +++--- bgpd/bgp_packet.c | 6 +++--- bgpd/bgpd.c | 14 +++++++------- bgpd/bgpd.h | 7 ++++--- tests/bgpd/test_aspath.c | 6 +++--- tests/bgpd/test_capability.c | 2 +- tests/bgpd/test_mp_attr.c | 2 +- 8 files changed, 26 insertions(+), 24 deletions(-) diff --git a/bgpd/bgp_dump.c b/bgpd/bgp_dump.c index 944a5848ec..299ee305be 100644 --- a/bgpd/bgp_dump.c +++ b/bgpd/bgp_dump.c @@ -389,7 +389,8 @@ bgp_dump_route_node_record(int afi, struct bgp_dest *dest, bgp_dump_routes_attr(obuf, path->attr, p); cur_endp = stream_get_endp(obuf); - if (cur_endp > BGP_MAX_PACKET_SIZE + BGP_DUMP_MSG_HEADER + if (cur_endp > BGP_STANDARD_MESSAGE_MAX_PACKET_SIZE + + BGP_DUMP_MSG_HEADER + BGP_DUMP_HEADER_SIZE) { stream_set_endp(obuf, endp); break; @@ -868,8 +869,8 @@ void bgp_dump_init(void) memset(&bgp_dump_routes, 0, sizeof(struct bgp_dump)); bgp_dump_obuf = - stream_new((BGP_MAX_PACKET_SIZE << 1) + BGP_DUMP_MSG_HEADER - + BGP_DUMP_HEADER_SIZE); + stream_new((BGP_STANDARD_MESSAGE_MAX_PACKET_SIZE * 2) + + BGP_DUMP_MSG_HEADER + BGP_DUMP_HEADER_SIZE); install_node(&bgp_dump_node); diff --git a/bgpd/bgp_open.c b/bgpd/bgp_open.c index 7642640218..94d905127d 100644 --- a/bgpd/bgp_open.c +++ b/bgpd/bgp_open.c @@ -1122,7 +1122,7 @@ int bgp_open_option_parse(struct peer *peer, uint8_t length, int *mp_capability) { int ret = 0; uint8_t *error; - uint8_t error_data[BGP_MAX_PACKET_SIZE]; + uint8_t error_data[BGP_STANDARD_MESSAGE_MAX_PACKET_SIZE]; struct stream *s = BGP_INPUT(peer); size_t end = stream_get_getp(s) + length; @@ -1217,8 +1217,8 @@ int bgp_open_option_parse(struct peer *peer, uint8_t length, int *mp_capability) /* Extended Message Support */ peer->max_packet_size = CHECK_FLAG(peer->cap, PEER_CAP_EXTENDED_MESSAGE_RCV) - ? BGP_MAX_EXTENDED_MESSAGE_PACKET_SIZE - : BGP_MAX_PACKET_SIZE; + ? BGP_EXTENDED_MESSAGE_MAX_PACKET_SIZE + : BGP_STANDARD_MESSAGE_MAX_PACKET_SIZE; /* Check there are no common AFI/SAFIs and send Unsupported Capability error. */ diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index d79621a749..62982881d8 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -545,7 +545,7 @@ void bgp_keepalive_send(struct peer *peer) { struct stream *s; - s = stream_new(BGP_MAX_PACKET_SIZE); + s = stream_new(BGP_STANDARD_MESSAGE_MAX_PACKET_SIZE); /* Make keepalive packet. */ bgp_packet_set_marker(s, BGP_MSG_KEEPALIVE); @@ -586,7 +586,7 @@ void bgp_open_send(struct peer *peer) else local_as = peer->local_as; - s = stream_new(BGP_MAX_PACKET_SIZE); + s = stream_new(BGP_STANDARD_MESSAGE_MAX_PACKET_SIZE); /* Make open packet. */ bgp_packet_set_marker(s, BGP_MSG_OPEN); @@ -752,7 +752,7 @@ void bgp_notify_send_with_data(struct peer *peer, uint8_t code, */ if (peer->curr) { size_t packetsize = stream_get_endp(peer->curr); - assert(packetsize <= sizeof(peer->last_reset_cause)); + assert(packetsize <= peer->max_packet_size); memcpy(peer->last_reset_cause, peer->curr->data, packetsize); peer->last_reset_cause_size = packetsize; } diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 73c1cbbbd9..21abfeb001 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -1349,7 +1349,7 @@ struct peer *peer_new(struct bgp *bgp) peer->bgp = bgp_lock(bgp); peer = peer_lock(peer); /* initial reference */ peer->password = NULL; - peer->max_packet_size = BGP_MAX_EXTENDED_MESSAGE_PACKET_SIZE; + peer->max_packet_size = BGP_MAX_PACKET_SIZE; /* Set default flags. */ FOREACH_AFI_SAFI (afi, safi) { @@ -1384,7 +1384,7 @@ struct peer *peer_new(struct bgp *bgp) /* We use a larger buffer for peer->obuf_work in the event that: * - We RX a BGP_UPDATE where the attributes alone are just - * under BGP_MAX_EXTENDED_MESSAGE_PACKET_SIZE. + * under BGP_EXTENDED_MESSAGE_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 @@ -1394,12 +1394,12 @@ struct peer *peer_new(struct bgp *bgp) * bounds checking for every single attribute as we construct an * UPDATE. */ - peer->obuf_work = stream_new(BGP_MAX_EXTENDED_MESSAGE_PACKET_SIZE - + BGP_MAX_PACKET_SIZE_OVERFLOW); - peer->ibuf_work = ringbuf_new(BGP_MAX_EXTENDED_MESSAGE_PACKET_SIZE - * BGP_READ_PACKET_MAX); + peer->obuf_work = + stream_new(BGP_MAX_PACKET_SIZE + BGP_MAX_PACKET_SIZE_OVERFLOW); + peer->ibuf_work = + ringbuf_new(BGP_MAX_PACKET_SIZE * BGP_READ_PACKET_MAX); - peer->scratch = stream_new(BGP_MAX_EXTENDED_MESSAGE_PACKET_SIZE); + peer->scratch = stream_new(BGP_MAX_PACKET_SIZE); bgp_sync_init(peer); diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h index 3343b332f4..38c6a70b8b 100644 --- a/bgpd/bgpd.h +++ b/bgpd/bgpd.h @@ -866,8 +866,9 @@ typedef enum { /* BGP message header and packet size. */ #define BGP_MARKER_SIZE 16 #define BGP_HEADER_SIZE 19 -#define BGP_MAX_PACKET_SIZE 4096 -#define BGP_MAX_EXTENDED_MESSAGE_PACKET_SIZE 65535 +#define BGP_STANDARD_MESSAGE_MAX_PACKET_SIZE 4096 +#define BGP_EXTENDED_MESSAGE_MAX_PACKET_SIZE 65535 +#define BGP_MAX_PACKET_SIZE BGP_EXTENDED_MESSAGE_MAX_PACKET_SIZE #define BGP_MAX_PACKET_SIZE_OVERFLOW 1024 /* @@ -1049,7 +1050,7 @@ struct peer { struct stream_fifo *obuf; // packets waiting to be written /* used as a block to deposit raw wire data to */ - uint8_t ibuf_scratch[BGP_MAX_EXTENDED_MESSAGE_PACKET_SIZE + uint8_t ibuf_scratch[BGP_EXTENDED_MESSAGE_MAX_PACKET_SIZE * BGP_READ_PACKET_MAX]; struct ringbuf *ibuf_work; // WiP buffer used by bgp_read() only struct stream *obuf_work; // WiP buffer used to construct packets diff --git a/tests/bgpd/test_aspath.c b/tests/bgpd/test_aspath.c index 1a9183c472..aaf3fd2aa4 100644 --- a/tests/bgpd/test_aspath.c +++ b/tests/bgpd/test_aspath.c @@ -892,7 +892,7 @@ static int validate(struct aspath *as, const struct test_spec *sp) /* Excercise AS4 parsing a bit, with a dogfood test */ if (!s) - s = stream_new(BGP_MAX_EXTENDED_MESSAGE_PACKET_SIZE); + s = stream_new(BGP_MAX_PACKET_SIZE); bytes4 = aspath_put(s, as, 1); as4 = make_aspath(STREAM_DATA(s), bytes4, 1); @@ -1201,13 +1201,13 @@ static int handle_attr_test(struct aspath_tests *t) asp = make_aspath(t->segment->asdata, t->segment->len, 0); - peer.curr = stream_new(BGP_MAX_EXTENDED_MESSAGE_PACKET_SIZE); + peer.curr = stream_new(BGP_MAX_PACKET_SIZE); peer.obuf = stream_fifo_new(); peer.bgp = &bgp; peer.host = (char *)"none"; peer.fd = -1; peer.cap = t->cap; - peer.max_packet_size = BGP_MAX_PACKET_SIZE; + peer.max_packet_size = BGP_STANDARD_MESSAGE_MAX_PACKET_SIZE; stream_write(peer.curr, t->attrheader, t->len); datalen = aspath_put(peer.curr, asp, t->as4 == AS4_DATA); diff --git a/tests/bgpd/test_capability.c b/tests/bgpd/test_capability.c index 91c0cce80c..153b83897d 100644 --- a/tests/bgpd/test_capability.c +++ b/tests/bgpd/test_capability.c @@ -935,7 +935,7 @@ int main(void) peer->afc_adv[i][j] = 1; } - peer->curr = stream_new(BGP_MAX_EXTENDED_MESSAGE_PACKET_SIZE); + peer->curr = stream_new(BGP_MAX_PACKET_SIZE); i = 0; while (mp_segments[i].name) diff --git a/tests/bgpd/test_mp_attr.c b/tests/bgpd/test_mp_attr.c index 8de0604c45..f510760913 100644 --- a/tests/bgpd/test_mp_attr.c +++ b/tests/bgpd/test_mp_attr.c @@ -1100,7 +1100,7 @@ int main(void) peer = peer_create_accept(bgp); peer->host = (char *)"foo"; peer->status = Established; - peer->curr = stream_new(BGP_MAX_EXTENDED_MESSAGE_PACKET_SIZE); + peer->curr = stream_new(BGP_MAX_PACKET_SIZE); ifp.ifindex = 0; peer->nexthop.ifp = &ifp;