From f6f434b2822c453f898552537180a812538bd19e Mon Sep 17 00:00:00 2001 From: Paul Jakma Date: Tue, 23 Nov 2010 21:28:03 +0000 Subject: bgpd: Try fix extcommunity resource allocation probs, particularly with 'set extcom..' * Extended communities has some kind of resource allocation problem which causes a double-free if the 'set extcommunity ...' command is used. Try fix by properly interning extcommunities. Also, more generally, make unintern functions take a double pointer so they can NULL out callers references - a usefully defensive programming pattern for functions which make refs invalid. Sadly, this patch doesn't fix the problem entirely - crashes still occur on session clear. * bgp_ecommunity.h: (ecommunity_{free,unintern}) take double pointer args. * bgp_community.h: (community_unintern) ditto * bgp_attr.h: (bgp_attr_intern) ditto * bgp_aspath.h: (bgp_aspath.h) ditto * (general) update all callers of above * bgp_routemap.c: (route_set_ecommunity_{rt,soo}) intern the new extcom added to the attr, and unintern any old one. (route_set_ecommunity_{rt,soo}_compile) intern the extcom to be used for the route-map set. (route_set_ecommunity_*_free) unintern to match, instead of free (route_set_ecommunity_soo) Do as _rt does and don't just leak any pre-existing community, add to it (is additive right though?) --- bgpd/bgp_packet.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'bgpd/bgp_packet.c') diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index 9102add7..dee0b51f 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -206,7 +206,7 @@ bgp_update_packet (struct peer *peer, afi_t afi, safi_t safi) /* Synchnorize attribute. */ if (adj->attr) - bgp_attr_unintern (adj->attr); + bgp_attr_unintern (&adj->attr); else peer->scount[afi][safi]++; @@ -1785,13 +1785,13 @@ bgp_update_receive (struct peer *peer, bgp_size_t size) /* Everything is done. We unintern temporary structures which interned in bgp_attr_parse(). */ if (attr.aspath) - aspath_unintern (attr.aspath); + aspath_unintern (&attr.aspath); if (attr.community) - community_unintern (attr.community); + community_unintern (&attr.community); if (attr.extra) { if (attr.extra->ecommunity) - ecommunity_unintern (attr.extra->ecommunity); + ecommunity_unintern (&attr.extra->ecommunity); if (attr.extra->cluster) cluster_unintern (attr.extra->cluster); if (attr.extra->transit) -- cgit v1.2.1 From b881c7074bb698aeb1b099175b325734fc6e44d2 Mon Sep 17 00:00:00 2001 From: Paul Jakma Date: Tue, 23 Nov 2010 16:35:42 +0000 Subject: bgpd: Implement revised error handling for partial optional/trans. attributes * BGP error handling generally boils down to "reset session". This was fine when all BGP speakers pretty much understood all BGP messages. However the increasing deployment of new attribute types has shown this approach to cause problems, in particular where a new attribute type is "tunneled" over some speakers which do not understand it, and then arrives at a speaker which does but considers it malformed (e.g. corruption along the way, or because of early implementation bugs/interop issues). To mitigate this drafts before the IDR (likely to be adopted) propose to treat errors in partial (i.e. not understood by neighbour), optional transitive attributes, when received from eBGP peers, as withdrawing only the NLRIs in the affected UPDATE, rather than causing the entire session to be reset. See: http://tools.ietf.org/html/draft-scudder-idr-optional-transitive * bgp_aspath.c: (assegments_parse) Replace the "NULL means valid, 0-length OR an error" return value with an error code - instead taking pointer to result structure as arg. (aspath_parse) adjust to suit previous change, but here NULL really does mean error in the external interface. * bgp_attr.h (bgp_attr_parse) use an explictly typed and enumerated value to indicate return result. (bgp_attr_unintern_sub) cleans up just the members of an attr, but not the attr itself, for benefit of those who use a stack-local attr. * bgp_attr.c: (bgp_attr_unintern_sub) split out from bgp_attr_unintern (bgp_attr_unintern) as previous. (bgp_attr_malformed) helper function to centralise decisions on how to handle errors in attributes. (bgp_attr_{aspathlimit,origin,etc..}) Use bgp_attr_malformed. (bgp_attr_aspathlimit) Subcode for error specifc to this attr should be BGP_NOTIFY_UPDATE_OPT_ATTR_ERR. (bgp_attr_as4_path) be more rigorous about checks, ala bgp_attr_as_path. (bgp_attr_parse) Adjust to deal with the additional error level that bgp_attr_ parsers can raise, and also similarly return appropriate error back up to (bgp_update_receive). Try to avoid leaking as4_path. * bgp_packet.c: (bgp_update_receive) Adjust to deal with BGP_ATTR_PARSE_WITHDRAW error level from bgp_attr_parse, which should lead to a withdraw, by making the attribute parameter in call to (bgp_nlri_parse) conditional on the error, so the update case morphs also into a withdraw. Use bgp_attr_unintern_sub from above, instead of doing this itself. Fix error case returns which were not calling bgp_attr_unintern_sub and probably leaking memory. * tests/aspath_test.c: Fix to work for null return with bad segments --- bgpd/bgp_packet.c | 76 +++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 48 insertions(+), 28 deletions(-) (limited to 'bgpd/bgp_packet.c') diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index dee0b51f..8de78c75 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -1589,26 +1589,47 @@ bgp_update_receive (struct peer *peer, bgp_size_t size) BGP_NOTIFY_UPDATE_MAL_ATTR); return -1; } + + /* Certain attribute parsing errors should not be considered bad enough + * to reset the session for, most particularly any partial/optional + * attributes that have 'tunneled' over speakers that don't understand + * them. Instead we withdraw only the prefix concerned. + * + * Complicates the flow a little though.. + */ + bgp_attr_parse_ret_t attr_parse_ret = BGP_ATTR_PARSE_PROCEED; + /* This define morphs the update case into a withdraw when lower levels + * have signalled an error condition where this is best. + */ +#define NLRI_ATTR_ARG (attr_parse_ret != BGP_ATTR_PARSE_WITHDRAW ? &attr : NULL) /* Parse attribute when it exists. */ if (attribute_len) { - ret = bgp_attr_parse (peer, &attr, attribute_len, + attr_parse_ret = bgp_attr_parse (peer, &attr, attribute_len, &mp_update, &mp_withdraw); - if (ret < 0) + if (attr_parse_ret == BGP_ATTR_PARSE_ERROR) return -1; } - + /* Logging the attribute. */ - if (BGP_DEBUG (update, UPDATE_IN)) + if (attr_parse_ret == BGP_ATTR_PARSE_WITHDRAW + || BGP_DEBUG (update, UPDATE_IN)) { ret= bgp_dump_attr (peer, &attr, attrstr, BUFSIZ); + int lvl = (attr_parse_ret == BGP_ATTR_PARSE_WITHDRAW) + ? LOG_ERR : LOG_DEBUG; + + if (attr_parse_ret == BGP_ATTR_PARSE_WITHDRAW) + zlog (peer->log, LOG_ERR, + "%s rcvd UPDATE with errors in attr(s)!! Withdrawing route.", + peer->host); if (ret) - zlog (peer->log, LOG_DEBUG, "%s rcvd UPDATE w/ attr: %s", + zlog (peer->log, lvl, "%s rcvd UPDATE w/ attr: %s", peer->host, attrstr); } - + /* Network Layer Reachability Information. */ update_len = end - stream_pnt (s); @@ -1617,7 +1638,12 @@ bgp_update_receive (struct peer *peer, bgp_size_t size) /* Check NLRI packet format and prefix length. */ ret = bgp_nlri_sanity_check (peer, AFI_IP, stream_pnt (s), update_len); if (ret < 0) - return -1; + { + bgp_attr_unintern_sub (&attr); + if (attr.extra) + bgp_attr_extra_free (&attr); + return -1; + } /* Set NLRI portion to structure. */ update.afi = AFI_IP; @@ -1640,15 +1666,20 @@ bgp_update_receive (struct peer *peer, bgp_size_t size) update. */ ret = bgp_attr_check (peer, &attr); if (ret < 0) - return -1; + { + bgp_attr_unintern_sub (&attr); + if (attr.extra) + bgp_attr_extra_free (&attr); + return -1; + } - bgp_nlri_parse (peer, &attr, &update); + bgp_nlri_parse (peer, NLRI_ATTR_ARG, &update); } if (mp_update.length && mp_update.afi == AFI_IP && mp_update.safi == SAFI_UNICAST) - bgp_nlri_parse (peer, &attr, &mp_update); + bgp_nlri_parse (peer, NLRI_ATTR_ARG, &mp_update); if (mp_withdraw.length && mp_withdraw.afi == AFI_IP @@ -1675,7 +1706,7 @@ bgp_update_receive (struct peer *peer, bgp_size_t size) if (mp_update.length && mp_update.afi == AFI_IP && mp_update.safi == SAFI_MULTICAST) - bgp_nlri_parse (peer, &attr, &mp_update); + bgp_nlri_parse (peer, NLRI_ATTR_ARG, &mp_update); if (mp_withdraw.length && mp_withdraw.afi == AFI_IP @@ -1705,7 +1736,7 @@ bgp_update_receive (struct peer *peer, bgp_size_t size) if (mp_update.length && mp_update.afi == AFI_IP6 && mp_update.safi == SAFI_UNICAST) - bgp_nlri_parse (peer, &attr, &mp_update); + bgp_nlri_parse (peer, NLRI_ATTR_ARG, &mp_update); if (mp_withdraw.length && mp_withdraw.afi == AFI_IP6 @@ -1734,7 +1765,7 @@ bgp_update_receive (struct peer *peer, bgp_size_t size) if (mp_update.length && mp_update.afi == AFI_IP6 && mp_update.safi == SAFI_MULTICAST) - bgp_nlri_parse (peer, &attr, &mp_update); + bgp_nlri_parse (peer, NLRI_ATTR_ARG, &mp_update); if (mp_withdraw.length && mp_withdraw.afi == AFI_IP6 @@ -1762,7 +1793,7 @@ bgp_update_receive (struct peer *peer, bgp_size_t size) if (mp_update.length && mp_update.afi == AFI_IP && mp_update.safi == BGP_SAFI_VPNV4) - bgp_nlri_parse_vpnv4 (peer, &attr, &mp_update); + bgp_nlri_parse_vpnv4 (peer, NLRI_ATTR_ARG, &mp_update); if (mp_withdraw.length && mp_withdraw.afi == AFI_IP @@ -1784,21 +1815,10 @@ bgp_update_receive (struct peer *peer, bgp_size_t size) /* Everything is done. We unintern temporary structures which interned in bgp_attr_parse(). */ - if (attr.aspath) - aspath_unintern (&attr.aspath); - if (attr.community) - community_unintern (&attr.community); + bgp_attr_unintern_sub (&attr); if (attr.extra) - { - if (attr.extra->ecommunity) - ecommunity_unintern (&attr.extra->ecommunity); - if (attr.extra->cluster) - cluster_unintern (attr.extra->cluster); - if (attr.extra->transit) - transit_unintern (attr.extra->transit); - bgp_attr_extra_free (&attr); - } - + bgp_attr_extra_free (&attr); + /* If peering is stopped due to some reason, do not generate BGP event. */ if (peer->status != Established) -- cgit v1.2.1 From 1212dc1961e81d5ef6e576b854e979ea29284f51 Mon Sep 17 00:00:00 2001 From: heasley Date: Mon, 12 Sep 2011 13:27:52 +0400 Subject: bgpd: add useful notification logs (BZ#616) * bgp_packet.c * bgp_notify_send_with_data(): add calls to zlog_info() --- bgpd/bgp_packet.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) (limited to 'bgpd/bgp_packet.c') diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index 1d9fcc97..2623cc29 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -895,12 +895,25 @@ bgp_notify_send_with_data (struct peer *peer, u_char code, u_char sub_code, if (sub_code != BGP_NOTIFY_CEASE_CONFIG_CHANGE) { if (sub_code == BGP_NOTIFY_CEASE_ADMIN_RESET) - peer->last_reset = PEER_DOWN_USER_RESET; + { + peer->last_reset = PEER_DOWN_USER_RESET; + zlog_info ("Notification sent to neighbor %s: User reset", peer->host); + } else if (sub_code == BGP_NOTIFY_CEASE_ADMIN_SHUTDOWN) - peer->last_reset = PEER_DOWN_USER_SHUTDOWN; + { + peer->last_reset = PEER_DOWN_USER_SHUTDOWN; + zlog_info ("Notification sent to neighbor %s: shutdown", peer->host); + } else - peer->last_reset = PEER_DOWN_NOTIFY_SEND; + { + peer->last_reset = PEER_DOWN_NOTIFY_SEND; + zlog_info ("Notification sent to neighbor %s: type %u/%u", + peer->host, code, sub_code); + } } + else + zlog_info ("Notification sent to neighbor %s: configuration change", + peer->host); /* Call imidiately. */ BGP_WRITE_OFF (peer->t_write); -- cgit v1.2.1 From 7ccf5e59c13773097dd551b8a7384b99b7f46927 Mon Sep 17 00:00:00 2001 From: Denis Ovsienko Date: Sat, 10 Sep 2011 16:53:30 +0400 Subject: bgpd: spelling --- bgpd/bgp_packet.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'bgpd/bgp_packet.c') diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index 2623cc29..ab0fa8f5 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -915,7 +915,7 @@ bgp_notify_send_with_data (struct peer *peer, u_char code, u_char sub_code, zlog_info ("Notification sent to neighbor %s: configuration change", peer->host); - /* Call imidiately. */ + /* Call immediately. */ BGP_WRITE_OFF (peer->t_write); bgp_write_notify (peer); -- cgit v1.2.1 From 42e6d745d105018a9469dabad65bd4cf942dcf3c Mon Sep 17 00:00:00 2001 From: Denis Ovsienko Date: Thu, 14 Jul 2011 12:36:19 +0400 Subject: bgpd: more SAFI fixes (with resolved conflict in bgpd/bgp_packet.c) Two macros resolving to the same integer constant broke a case block and a more thorough merge of BGP_SAFI_VPNV4 and BGP_SAFI_VPNV6 was performed. * bgpd.h: MPLS-labeled VPN SAFI is AFI-independent, switch to single * macro * bgp_capability_test.c: update test data * bgp_mp_attr_test.c: idem * bgp_route.c: (bgp_maximum_prefix_overflow, bgp_table_stats_vty) update macro and check conditions (where appropriate) * bgp_packet.c: (bgp_route_refresh_send, bgp_capability_send, bgp_update_receive, bgp_route_refresh_receive): idem * bgp_open.c: (bgp_capability_vty_out, bgp_afi_safi_valid_indices, bgp_open_capability_orf, bgp_open_capability): idem * bgp_attr.c: (bgp_mp_reach_parse, bgp_packet_attribute, bgp_packet_withdraw): idem --- bgpd/bgp_packet.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'bgpd/bgp_packet.c') diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index ab0fa8f5..4854f1dd 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -946,7 +946,7 @@ bgp_route_refresh_send (struct peer *peer, afi_t afi, safi_t safi, /* Adjust safi code. */ if (safi == SAFI_MPLS_VPN) - safi = BGP_SAFI_VPNV4; + safi = SAFI_MPLS_LABELED_VPN; s = stream_new (BGP_MAX_PACKET_SIZE); @@ -1036,7 +1036,7 @@ bgp_capability_send (struct peer *peer, afi_t afi, safi_t safi, /* Adjust safi code. */ if (safi == SAFI_MPLS_VPN) - safi = BGP_SAFI_VPNV4; + safi = SAFI_MPLS_LABELED_VPN; s = stream_new (BGP_MAX_PACKET_SIZE); @@ -1799,17 +1799,17 @@ bgp_update_receive (struct peer *peer, bgp_size_t size) { if (mp_update.length && mp_update.afi == AFI_IP - && mp_update.safi == BGP_SAFI_VPNV4) + && mp_update.safi == SAFI_MPLS_LABELED_VPN) bgp_nlri_parse_vpnv4 (peer, NLRI_ATTR_ARG, &mp_update); if (mp_withdraw.length && mp_withdraw.afi == AFI_IP - && mp_withdraw.safi == BGP_SAFI_VPNV4) + && mp_withdraw.safi == SAFI_MPLS_LABELED_VPN) bgp_nlri_parse_vpnv4 (peer, NULL, &mp_withdraw); if (! withdraw_len && mp_withdraw.afi == AFI_IP - && mp_withdraw.safi == BGP_SAFI_VPNV4 + && mp_withdraw.safi == SAFI_MPLS_LABELED_VPN && mp_withdraw.length == 0) { /* End-of-RIB received */ @@ -1969,7 +1969,7 @@ bgp_route_refresh_receive (struct peer *peer, bgp_size_t size) /* Check AFI and SAFI. */ if ((afi != AFI_IP && afi != AFI_IP6) || (safi != SAFI_UNICAST && safi != SAFI_MULTICAST - && safi != BGP_SAFI_VPNV4)) + && safi != SAFI_MPLS_LABELED_VPN)) { if (BGP_DEBUG (normal, NORMAL)) { @@ -1980,7 +1980,7 @@ bgp_route_refresh_receive (struct peer *peer, bgp_size_t size) } /* Adjust safi code. */ - if (safi == BGP_SAFI_VPNV4) + if (safi == SAFI_MPLS_LABELED_VPN) safi = SAFI_MPLS_VPN; if (size != BGP_MSG_ROUTE_REFRESH_MIN_SIZE - BGP_HEADER_SIZE) -- cgit v1.2.1 From bb915f5fa60de1a5b7e6089fcfc680281a590463 Mon Sep 17 00:00:00 2001 From: Denis Ovsienko Date: Tue, 13 Dec 2011 21:11:39 +0400 Subject: bgpd: fix regression in ORF procesing (BZ#688) This issue has been pointed out by Lou Berger and Tim Browski. * bgp_packet.c * bgp_route_refresh_receive(): restore if() condition, which was broken by commit fdbc8e77c88f751924299d0bc752371d5cc31116 --- bgpd/bgp_packet.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'bgpd/bgp_packet.c') diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index 4854f1dd..2c0113da 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -2054,7 +2054,7 @@ bgp_route_refresh_receive (struct peer *peer, bgp_size_t size) break; } ok = ((p_end - p_pnt) >= sizeof(u_int32_t)) ; - if (!ok) + if (ok) { memcpy (&seq, p_pnt, sizeof (u_int32_t)); p_pnt += sizeof (u_int32_t); -- cgit v1.2.1 From 733cd9e5792648de50da3c00805aacb51cb27048 Mon Sep 17 00:00:00 2001 From: Denis Ovsienko Date: Sat, 17 Dec 2011 19:39:30 +0400 Subject: bgpd: justify checks for IPv4 class D/E * lib/prefix.h * IPV4_CLASS_DE(): make consistent with counterpart macros * bgp_packet.c * bgp_open_receive(): test using macro instead of ">=" * bgp_route.c * bgp_update_rsclient(): idem * bgp_update_main(): idem --- bgpd/bgp_packet.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'bgpd/bgp_packet.c') diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index 2c0113da..f5a74d1b 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -1381,7 +1381,7 @@ bgp_open_receive (struct peer *peer, bgp_size_t size) /* remote router-id check. */ if (remote_id.s_addr == 0 - || ntohl (remote_id.s_addr) >= 0xe0000000 + || IPV4_CLASS_DE (ntohl (remote_id.s_addr)) || ntohl (peer->local_id.s_addr) == ntohl (remote_id.s_addr)) { if (BGP_DEBUG (normal, NORMAL)) -- cgit v1.2.1 From 5861739f8c38bc36ea9955e5cb2be2bf2f482d70 Mon Sep 17 00:00:00 2001 From: Paul Jakma Date: Mon, 9 Jan 2012 20:59:26 +0000 Subject: bgpd: Open option parse errors don't NOTIFY, resulting in abort & DoS * bgp_packet.c: (bgp_open_receive) Errors from bgp_open_option_parse are detected, and the code will stop processing the OPEN and return. However it does so without calling bgp_notify_send to send a NOTIFY - which means the peer FSM doesn't get stopped, and bgp_read will be called again later. Because it returns, it doesn't go through the code near the end of the function that removes the current message from the peer input streaam. Thus the next call to bgp_read will try to parse a half-parsed stream as if it were a new BGP message, leading to an assert later in the code when it tries to read stuff that isn't there. Add the required call to bgp_notify_send before returning. * bgp_open.c: (bgp_capability_as4) Be a bit stricter, check the length field corresponds to the only value it can be, which is the amount we're going to read off the stream. And make sure the capability flag gets set, so callers can know this capability was read, regardless. (peek_for_as4_capability) Let bgp_capability_as4 do the length check. --- bgpd/bgp_packet.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) (limited to 'bgpd/bgp_packet.c') diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c index f5a74d1b..5d8087a8 100644 --- a/bgpd/bgp_packet.c +++ b/bgpd/bgp_packet.c @@ -1459,9 +1459,13 @@ bgp_open_receive (struct peer *peer, bgp_size_t size) /* Open option part parse. */ if (optlen != 0) { - ret = bgp_open_option_parse (peer, optlen, &capability); - if (ret < 0) - return ret; + if ((ret = bgp_open_option_parse (peer, optlen, &capability)) < 0) + { + bgp_notify_send (peer, + BGP_NOTIFY_OPEN_ERR, + BGP_NOTIFY_OPEN_UNACEP_HOLDTIME); + return ret; + } } else { -- cgit v1.2.1