From ab005298526f4b14126cae1a6461ad3d700af29c Mon Sep 17 00:00:00 2001 From: Paul Jakma Date: Sat, 27 Nov 2010 22:48:34 +0000 Subject: bgpd: Rollback some of the changes made for invalid AS_PATH segment fix Some of the changes made in commit cddb8112b80fa9867156c637d63e6e79eeac67bb don't work particularly well for other changes that need to be made to address BGP attribute error handling problems. In particular, returning a pointer from complex attribute data parsing functions will not suffice to express the require range of return status conditions. * bgp_aspath.c: (assegments_parse) Rollback to a more minimal set of changes to fix the original problem. (aspath_parse) Slightly needless pushing around of code, and taking 2 parameters to say whether ot use 2 or 4 byte encoding seems unnecessary. * bgp_attr.c: (bgp_attr_as{,4}path) Rollback, in preparation for BGP attribute error handling update. --- bgpd/bgp_attr.c | 100 ++++++++++++++++++++++++-------------------------------- 1 file changed, 43 insertions(+), 57 deletions(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index e2b6054b..7a0ab560 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -746,78 +746,51 @@ bgp_attr_origin (struct peer *peer, bgp_size_t length, return 0; } -/* Parse AS path information. This function is wrapper of aspath_parse. - * - * Parses AS_PATH or AS4_PATH. - * - * Returns: if valid: address of struct aspath in the hash of known aspaths, - * with reference count incremented. - * else: NULL - * - * NB: empty AS path (length == 0) is valid. The returned struct aspath will - * have segments == NULL and str == zero length string (unique). - */ -static struct aspath * + +/* Parse AS path information. This function is wrapper of + aspath_parse. */ +static int bgp_attr_aspath (struct peer *peer, bgp_size_t length, - struct attr *attr, u_char flag, u_char *startp, int as4_path) + struct attr *attr, u_char flag, u_char *startp) { - u_char require ; - struct aspath *asp ; + bgp_size_t total; - /* Check the attribute flags */ - require = as4_path ? BGP_ATTR_FLAG_OPTIONAL | BGP_ATTR_FLAG_TRANS - : BGP_ATTR_FLAG_TRANS ; + total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); - if ((flag & (BGP_ATTR_FLAG_OPTIONAL | BGP_ATTR_FLAG_TRANS)) != require) + /* Flag check. */ + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL) + || ! CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) { - const char* path_type ; - bgp_size_t total; - - path_type = as4_path ? "AS4_PATH" : "AS_PATH" ; - - if (!CHECK_FLAG(flag, BGP_ATTR_FLAG_TRANS)) zlog (peer->log, LOG_ERR, - "%s attribute flag isn't transitive %d", path_type, flag) ; - - if ((flag & BGP_ATTR_FLAG_OPTIONAL) != (require & BGP_ATTR_FLAG_OPTIONAL)) - zlog (peer->log, LOG_ERR, - "%s attribute flag must %sbe optional %d", path_type, - (flag & BGP_ATTR_FLAG_OPTIONAL) ? "not " : "", flag) ; - - total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); - + "As-Path attribute flag isn't transitive %d", flag); bgp_notify_send_with_data (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); + return -1; + } - return NULL ; - } ; - - /* Parse the AS_PATH/AS4_PATH body. - * - * For AS_PATH peer with AS4 => 4Byte ASN otherwise 2Byte ASN - * AS4_PATH 4Byte ASN + /* + * peer with AS4 => will get 4Byte ASnums + * otherwise, will get 16 Bit */ - asp = aspath_parse (peer->ibuf, length, - as4_path || CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV), as4_path) ; + attr->aspath = aspath_parse (peer->ibuf, length, + CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV)); - if (asp != NULL) - { - attr->flag |= ATTR_FLAG_BIT (as4_path ? BGP_ATTR_AS4_PATH - : BGP_ATTR_AS_PATH) ; - } - else + /* In case of IBGP, length will be zero. */ + if (! attr->aspath) { zlog (peer->log, LOG_ERR, "Malformed AS path length is %d", length); - - /* TODO: should BGP_NOTIFY_UPDATE_MAL_AS_PATH be sent for AS4_PATH ?? */ bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_MAL_AS_PATH); - } ; + return -1; + } + + /* Set aspath attribute flag. */ + attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_AS_PATH); - return asp ; + return 0; } static int bgp_attr_aspath_check( struct peer *peer, @@ -875,6 +848,21 @@ static int bgp_attr_aspath_check( struct peer *peer, } +/* Parse AS4 path information. This function is another wrapper of + aspath_parse. */ +static int +bgp_attr_as4_path (struct peer *peer, bgp_size_t length, + struct attr *attr, struct aspath **as4_path) +{ + *as4_path = aspath_parse (peer->ibuf, length, 1); + + /* Set aspath attribute flag. */ + if (as4_path) + attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_AS4_PATH); + + return 0; +} + /* Nexthop attribute. */ static int bgp_attr_nexthop (struct peer *peer, bgp_size_t length, @@ -1613,12 +1601,10 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, ret = bgp_attr_origin (peer, length, attr, flag, startp); break; case BGP_ATTR_AS_PATH: - attr->aspath = bgp_attr_aspath (peer, length, attr, flag, startp, 0); - ret = attr->aspath ? 0 : -1 ; + ret = bgp_attr_aspath (peer, length, attr, flag, startp); break; case BGP_ATTR_AS4_PATH: - as4_path = bgp_attr_aspath (peer, length, attr, flag, startp, 1); - ret = as4_path ? 0 : -1 ; + ret = bgp_attr_as4_path (peer, length, attr, &as4_path ); break; case BGP_ATTR_NEXT_HOP: ret = bgp_attr_nexthop (peer, length, attr, flag, startp); -- cgit v1.2.1 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_attr.c | 54 ++++++++++++++++++++++++++++-------------------------- 1 file changed, 28 insertions(+), 26 deletions(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index 7a0ab560..0c5355b4 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -500,6 +500,7 @@ bgp_attr_intern (struct attr *attr) attre->ecommunity = ecommunity_intern (attre->ecommunity); else attre->ecommunity->refcnt++; + } if (attre->cluster) { @@ -516,10 +517,10 @@ bgp_attr_intern (struct attr *attr) attre->transit->refcnt++; } } - + find = (struct attr *) hash_get (attrhash, attr, bgp_attr_hash_alloc); find->refcnt++; - + return find; } @@ -561,7 +562,7 @@ bgp_attr_default_intern (u_char origin) new = bgp_attr_intern (&attr); bgp_attr_extra_free (&attr); - aspath_unintern (new->aspath); + aspath_unintern (&new->aspath); return new; } @@ -613,13 +614,13 @@ bgp_attr_aggregate_intern (struct bgp *bgp, u_char origin, new = bgp_attr_intern (&attr); bgp_attr_extra_free (&attr); - aspath_unintern (new->aspath); + aspath_unintern (&new->aspath); return new; } /* Free bgp attribute and aspath. */ void -bgp_attr_unintern (struct attr *attr) +bgp_attr_unintern (struct attr **attr) { struct attr *ret; struct aspath *aspath; @@ -627,34 +628,35 @@ bgp_attr_unintern (struct attr *attr) struct ecommunity *ecommunity = NULL; struct cluster_list *cluster = NULL; struct transit *transit = NULL; - + /* Decrement attribute reference. */ - attr->refcnt--; - aspath = attr->aspath; - community = attr->community; - if (attr->extra) + (*attr)->refcnt--; + aspath = (*attr)->aspath; + community = (*attr)->community; + if ((*attr)->extra) { - ecommunity = attr->extra->ecommunity; - cluster = attr->extra->cluster; - transit = attr->extra->transit; + ecommunity = (*attr)->extra->ecommunity; + cluster = (*attr)->extra->cluster; + transit = (*attr)->extra->transit; } - + /* If reference becomes zero then free attribute object. */ - if (attr->refcnt == 0) + if ((*attr)->refcnt == 0) { - ret = hash_release (attrhash, attr); + ret = hash_release (attrhash, *attr); assert (ret != NULL); - bgp_attr_extra_free (attr); - XFREE (MTYPE_ATTR, attr); + bgp_attr_extra_free (*attr); + XFREE (MTYPE_ATTR, *attr); + *attr = NULL; } /* aspath refcount shoud be decrement. */ if (aspath) - aspath_unintern (aspath); + aspath_unintern (&aspath); if (community) - community_unintern (community); + community_unintern (&community); if (ecommunity) - ecommunity_unintern (ecommunity); + ecommunity_unintern (&ecommunity); if (cluster) cluster_unintern (cluster); if (transit) @@ -671,8 +673,9 @@ bgp_attr_flush (struct attr *attr) if (attr->extra) { struct attr_extra *attre = attr->extra; + if (attre->ecommunity && ! attre->ecommunity->refcnt) - ecommunity_free (attre->ecommunity); + ecommunity_free (&attre->ecommunity); if (attre->cluster && ! attre->cluster->refcnt) cluster_free (attre->cluster); if (attre->transit && ! attre->transit->refcnt) @@ -840,7 +843,7 @@ static int bgp_attr_aspath_check( struct peer *peer, { aspath = aspath_dup (attr->aspath); aspath = aspath_add_seq (aspath, peer->change_local_as); - aspath_unintern (attr->aspath); + aspath_unintern (&attr->aspath); attr->aspath = aspath_intern (aspath); } @@ -1146,7 +1149,7 @@ bgp_attr_munge_as4_attrs (struct peer *peer, struct attr *attr, if ( !ignore_as4_path && (attr->flag & ( ATTR_FLAG_BIT( BGP_ATTR_AS4_PATH))) ) { newpath = aspath_reconcile_as4 (attr->aspath, as4_path); - aspath_unintern (attr->aspath); + aspath_unintern (&attr->aspath); attr->aspath = aspath_intern (newpath); } return 0; @@ -1707,8 +1710,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, */ if ( as4_path ) { - aspath_unintern( as4_path ); /* unintern - it is in the hash */ - as4_path = NULL; + aspath_unintern (&as4_path); /* unintern - it is in the hash */ /* The flag that we got this is still there, but that does not * do any trouble */ -- 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_attr.c | 497 ++++++++++++++++++++++++++++++++++---------------------- 1 file changed, 304 insertions(+), 193 deletions(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index 0c5355b4..6eab5ae8 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -618,26 +618,50 @@ bgp_attr_aggregate_intern (struct bgp *bgp, u_char origin, return new; } +/* Unintern just the sub-components of the attr, but not the attr */ +void +bgp_attr_unintern_sub (struct attr *attr) +{ + /* aspath refcount shoud be decrement. */ + if (attr->aspath) + aspath_unintern (&attr->aspath); + UNSET_FLAG(attr->flag, BGP_ATTR_AS_PATH); + + if (attr->community) + community_unintern (&attr->community); + UNSET_FLAG(attr->flag, BGP_ATTR_COMMUNITIES); + + if (attr->extra) + { + if (attr->extra->ecommunity) + ecommunity_unintern (&attr->extra->ecommunity); + UNSET_FLAG(attr->flag, BGP_ATTR_EXT_COMMUNITIES); + + if (attr->extra->cluster) + cluster_unintern (attr->extra->cluster); + UNSET_FLAG(attr->flag, BGP_ATTR_CLUSTER_LIST); + + if (attr->extra->transit) + transit_unintern (attr->extra->transit); + } +} + /* Free bgp attribute and aspath. */ void bgp_attr_unintern (struct attr **attr) { struct attr *ret; - struct aspath *aspath; - struct community *community; - struct ecommunity *ecommunity = NULL; - struct cluster_list *cluster = NULL; - struct transit *transit = NULL; + struct attr tmp; /* Decrement attribute reference. */ (*attr)->refcnt--; - aspath = (*attr)->aspath; - community = (*attr)->community; + + tmp = *(*attr); + if ((*attr)->extra) { - ecommunity = (*attr)->extra->ecommunity; - cluster = (*attr)->extra->cluster; - transit = (*attr)->extra->transit; + tmp.extra = bgp_attr_extra_new (); + memcpy (tmp.extra, (*attr)->extra, sizeof (struct attr_extra)); } /* If reference becomes zero then free attribute object. */ @@ -650,17 +674,7 @@ bgp_attr_unintern (struct attr **attr) *attr = NULL; } - /* aspath refcount shoud be decrement. */ - if (aspath) - aspath_unintern (&aspath); - if (community) - community_unintern (&community); - if (ecommunity) - ecommunity_unintern (&ecommunity); - if (cluster) - cluster_unintern (cluster); - if (transit) - transit_unintern (transit); + bgp_attr_unintern_sub (&tmp); } void @@ -683,8 +697,69 @@ bgp_attr_flush (struct attr *attr) } } +/* Implement draft-scudder-idr-optional-transitive behaviour and + * avoid resetting sessions for malformed attributes which are + * are partial/optional and hence where the error likely was not + * introduced by the sending neighbour. + */ +static bgp_attr_parse_ret_t +bgp_attr_malformed (struct peer *peer, u_char type, u_char flag, + u_char subcode, u_char *startp, bgp_size_t length) +{ + /* Only relax error handling for eBGP peers */ + if (peer_sort (peer) != BGP_PEER_EBGP) + { + bgp_notify_send_with_data (peer, BGP_NOTIFY_UPDATE_ERR, subcode, + startp, length); + return BGP_ATTR_PARSE_ERROR; + + } + + switch (type) { + /* where an optional attribute is inconsequential, e.g. it does not affect + * route selection, and can be safely ignored then any such attributes + * which are malformed should just be ignored and the route processed as + * normal. + */ + case BGP_ATTR_AS4_AGGREGATOR: + case BGP_ATTR_AGGREGATOR: + case BGP_ATTR_ATOMIC_AGGREGATE: + return BGP_ATTR_PARSE_PROCEED; + + /* Core attributes, particularly ones which may influence route + * selection should always cause session resets + */ + case BGP_ATTR_ORIGIN: + case BGP_ATTR_AS_PATH: + case BGP_ATTR_NEXT_HOP: + case BGP_ATTR_MULTI_EXIT_DISC: + case BGP_ATTR_LOCAL_PREF: + case BGP_ATTR_COMMUNITIES: + case BGP_ATTR_ORIGINATOR_ID: + case BGP_ATTR_CLUSTER_LIST: + case BGP_ATTR_MP_REACH_NLRI: + case BGP_ATTR_MP_UNREACH_NLRI: + case BGP_ATTR_EXT_COMMUNITIES: + bgp_notify_send_with_data (peer, BGP_NOTIFY_UPDATE_ERR, subcode, + startp, length); + return BGP_ATTR_PARSE_ERROR; + } + + /* Partial optional attributes that are malformed should not cause + * the whole session to be reset. Instead treat it as a withdrawal + * of the routes, if possible. + */ + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS) + && CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL) + && CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) + return BGP_ATTR_PARSE_WITHDRAW; + + /* default to reset */ + return BGP_ATTR_PARSE_ERROR; +} + /* Get origin attribute of the update message. */ -static int +static bgp_attr_parse_ret_t bgp_attr_origin (struct peer *peer, bgp_size_t length, struct attr *attr, u_char flag, u_char *startp) { @@ -702,11 +777,9 @@ bgp_attr_origin (struct peer *peer, bgp_size_t length, { zlog (peer->log, LOG_ERR, "Origin attribute flag isn't transitive %d", flag); - bgp_notify_send_with_data (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_ORIGIN, flag, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); } /* If any recognized attribute has Attribute Length that conflicts @@ -718,10 +791,9 @@ bgp_attr_origin (struct peer *peer, bgp_size_t length, { zlog (peer->log, LOG_ERR, "Origin attribute length is not one %d", length); - bgp_notify_send_with_data (peer, BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, - startp, total); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_ORIGIN, flag, + BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, + startp, total); } /* Fetch origin attribute. */ @@ -736,12 +808,9 @@ bgp_attr_origin (struct peer *peer, bgp_size_t length, { zlog (peer->log, LOG_ERR, "Origin attribute value is invalid %d", attr->origin); - - bgp_notify_send_with_data (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_INVAL_ORIGIN, - startp, total); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_ORIGIN, flag, + BGP_NOTIFY_UPDATE_INVAL_ORIGIN, + startp, total); } /* Set oring attribute flag. */ @@ -766,11 +835,9 @@ bgp_attr_aspath (struct peer *peer, bgp_size_t length, { zlog (peer->log, LOG_ERR, "As-Path attribute flag isn't transitive %d", flag); - bgp_notify_send_with_data (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); } /* @@ -783,21 +850,22 @@ bgp_attr_aspath (struct peer *peer, bgp_size_t length, /* In case of IBGP, length will be zero. */ if (! attr->aspath) { - zlog (peer->log, LOG_ERR, "Malformed AS path length is %d", length); - bgp_notify_send (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_MAL_AS_PATH); - return -1; + zlog (peer->log, LOG_ERR, + "Malformed AS path from %s, length is %d", + peer->host, length); + return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag, + BGP_NOTIFY_UPDATE_MAL_AS_PATH, + NULL, 0); } /* Set aspath attribute flag. */ attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_AS_PATH); - return 0; + return BGP_ATTR_PARSE_PROCEED; } -static int bgp_attr_aspath_check( struct peer *peer, - struct attr *attr) +static bgp_attr_parse_ret_t +bgp_attr_aspath_check (struct peer *peer, struct attr *attr, u_char flag) { /* These checks were part of bgp_attr_aspath, but with * as4 we should to check aspath things when @@ -816,10 +884,9 @@ static int bgp_attr_aspath_check( struct peer *peer, (peer_sort (peer) == BGP_PEER_EBGP && aspath_confed_check (attr->aspath))) { zlog (peer->log, LOG_ERR, "Malformed AS path from %s", peer->host); - bgp_notify_send (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_MAL_AS_PATH); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag, + BGP_NOTIFY_UPDATE_MAL_AS_PATH, + NULL, 0); } /* First AS check for EBGP. */ @@ -830,10 +897,9 @@ static int bgp_attr_aspath_check( struct peer *peer, { zlog (peer->log, LOG_ERR, "%s incorrect first AS (must be %u)", peer->host, peer->as); - bgp_notify_send (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_MAL_AS_PATH); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag, + BGP_NOTIFY_UPDATE_MAL_AS_PATH, + NULL, 0); } } @@ -847,27 +913,53 @@ static int bgp_attr_aspath_check( struct peer *peer, attr->aspath = aspath_intern (aspath); } - return 0; - + return BGP_ATTR_PARSE_PROCEED; } /* Parse AS4 path information. This function is another wrapper of aspath_parse. */ static int -bgp_attr_as4_path (struct peer *peer, bgp_size_t length, - struct attr *attr, struct aspath **as4_path) +bgp_attr_as4_path (struct peer *peer, bgp_size_t length, + struct attr *attr, u_char flag, u_char *startp, + struct aspath **as4_path) { + bgp_size_t total; + + total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); + + /* Flag check. */ + if (!CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL) + || !CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) + { + zlog (peer->log, LOG_ERR, + "As4-Path attribute flag isn't optional/transitive %d", flag); + return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); + } + *as4_path = aspath_parse (peer->ibuf, length, 1); + /* In case of IBGP, length will be zero. */ + if (!*as4_path) + { + zlog (peer->log, LOG_ERR, + "Malformed AS4 path from %s, length is %d", + peer->host, length); + return bgp_attr_malformed (peer, BGP_ATTR_AS4_PATH, flag, + BGP_NOTIFY_UPDATE_MAL_AS_PATH, + NULL, 0); + } + /* Set aspath attribute flag. */ if (as4_path) attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_AS4_PATH); - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* Nexthop attribute. */ -static int +static bgp_attr_parse_ret_t bgp_attr_nexthop (struct peer *peer, bgp_size_t length, struct attr *attr, u_char flag, u_char *startp) { @@ -881,11 +973,9 @@ bgp_attr_nexthop (struct peer *peer, bgp_size_t length, { zlog (peer->log, LOG_ERR, "Origin attribute flag isn't transitive %d", flag); - bgp_notify_send_with_data (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_NEXT_HOP, flag, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); } /* Check nexthop attribute length. */ @@ -894,21 +984,19 @@ bgp_attr_nexthop (struct peer *peer, bgp_size_t length, zlog (peer->log, LOG_ERR, "Nexthop attribute length isn't four [%d]", length); - bgp_notify_send_with_data (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, - startp, total); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_NEXT_HOP, flag, + BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, + startp, total); } attr->nexthop.s_addr = stream_get_ipv4 (peer->ibuf); attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_NEXT_HOP); - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* MED atrribute. */ -static int +static bgp_attr_parse_ret_t bgp_attr_med (struct peer *peer, bgp_size_t length, struct attr *attr, u_char flag, u_char *startp) { @@ -921,23 +1009,21 @@ bgp_attr_med (struct peer *peer, bgp_size_t length, { zlog (peer->log, LOG_ERR, "MED attribute length isn't four [%d]", length); - - bgp_notify_send_with_data (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, - startp, total); - return -1; + + return bgp_attr_malformed (peer, BGP_ATTR_MULTI_EXIT_DISC, flag, + BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, + startp, total); } attr->med = stream_getl (peer->ibuf); attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_MULTI_EXIT_DISC); - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* Local preference attribute. */ -static int +static bgp_attr_parse_ret_t bgp_attr_local_pref (struct peer *peer, bgp_size_t length, struct attr *attr, u_char flag) { @@ -947,7 +1033,7 @@ bgp_attr_local_pref (struct peer *peer, bgp_size_t length, if (peer_sort (peer) == BGP_PEER_EBGP) { stream_forward_getp (peer->ibuf, length); - return 0; + return BGP_ATTR_PARSE_PROCEED; } if (length == 4) @@ -958,7 +1044,7 @@ bgp_attr_local_pref (struct peer *peer, bgp_size_t length, /* Set atomic aggregate flag. */ attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_LOCAL_PREF); - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* Atomic aggregate. */ @@ -970,16 +1056,15 @@ bgp_attr_atomic (struct peer *peer, bgp_size_t length, { zlog (peer->log, LOG_ERR, "Bad atomic aggregate length %d", length); - bgp_notify_send (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_LENG_ERR); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_ATOMIC_AGGREGATE, flag, + BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, + NULL, 0); } /* Set atomic aggregate flag. */ attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_ATOMIC_AGGREGATE); - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* Aggregator attribute */ @@ -991,17 +1076,16 @@ bgp_attr_aggregator (struct peer *peer, bgp_size_t length, struct attr_extra *attre = bgp_attr_extra_get (attr); /* peer with AS4 will send 4 Byte AS, peer without will send 2 Byte */ - if ( CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV ) ) + if (CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV)) wantedlen = 8; if (length != wantedlen) { zlog (peer->log, LOG_ERR, "Aggregator length is not %d [%d]", wantedlen, length); - bgp_notify_send (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_LENG_ERR); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_AGGREGATOR, flag, + BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, + NULL, 0); } if ( CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV ) ) @@ -1013,36 +1097,35 @@ bgp_attr_aggregator (struct peer *peer, bgp_size_t length, /* Set atomic aggregate flag. */ attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_AGGREGATOR); - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* New Aggregator attribute */ -static int +static bgp_attr_parse_ret_t bgp_attr_as4_aggregator (struct peer *peer, bgp_size_t length, - struct attr *attr, as_t *as4_aggregator_as, + struct attr *attr, u_char flag, + as_t *as4_aggregator_as, struct in_addr *as4_aggregator_addr) { if (length != 8) { zlog (peer->log, LOG_ERR, "New Aggregator length is not 8 [%d]", length); - - bgp_notify_send (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_LENG_ERR); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_AS4_AGGREGATOR, flag, + BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, + NULL, 0); } *as4_aggregator_as = stream_getl (peer->ibuf); as4_aggregator_addr->s_addr = stream_get_ipv4 (peer->ibuf); attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_AS4_AGGREGATOR); - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* Munge Aggregator and New-Aggregator, AS_PATH and NEW_AS_PATH. */ -static int -bgp_attr_munge_as4_attrs (struct peer *peer, struct attr *attr, +static bgp_attr_parse_ret_t +bgp_attr_munge_as4_attrs (struct peer *peer, struct attr *attr, u_char flag, struct aspath *as4_path, as_t as4_aggregator, struct in_addr *as4_aggregator_addr) { @@ -1050,7 +1133,7 @@ bgp_attr_munge_as4_attrs (struct peer *peer, struct attr *attr, struct aspath *newpath; struct attr_extra *attre = attr->extra; - if ( CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV) ) + if (CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV)) { /* peer can do AS4, so we ignore AS4_PATH and AS4_AGGREGATOR * if given. @@ -1068,11 +1151,11 @@ bgp_attr_munge_as4_attrs (struct peer *peer, struct attr *attr, peer->host, "AS4 capable peer, yet it sent"); } - return 0; + return BGP_ATTR_PARSE_PROCEED; } - if (attr->flag & ( ATTR_FLAG_BIT( BGP_ATTR_AS4_PATH)) - && !(attr->flag & ( ATTR_FLAG_BIT( BGP_ATTR_AS_PATH)))) + if (attr->flag & (ATTR_FLAG_BIT (BGP_ATTR_AS4_PATH)) + && !(attr->flag & (ATTR_FLAG_BIT (BGP_ATTR_AS_PATH)))) { /* Hu? This is not supposed to happen at all! * got as4_path and no aspath, @@ -1084,10 +1167,9 @@ bgp_attr_munge_as4_attrs (struct peer *peer, struct attr *attr, zlog (peer->log, LOG_ERR, "%s BGP not AS4 capable peer sent AS4_PATH but" " no AS_PATH, cant do anything here", peer->host); - bgp_notify_send (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_MAL_ATTR); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag, + BGP_NOTIFY_UPDATE_MAL_ATTR, + NULL, 0); } /* We have a asn16 peer. First, look for AS4_AGGREGATOR @@ -1095,7 +1177,7 @@ bgp_attr_munge_as4_attrs (struct peer *peer, struct attr *attr, */ if (attr->flag & (ATTR_FLAG_BIT (BGP_ATTR_AS4_AGGREGATOR) ) ) { - if ( attr->flag & (ATTR_FLAG_BIT (BGP_ATTR_AGGREGATOR) ) ) + if (attr->flag & (ATTR_FLAG_BIT (BGP_ATTR_AGGREGATOR) ) ) { assert (attre); @@ -1111,7 +1193,7 @@ bgp_attr_munge_as4_attrs (struct peer *peer, struct attr *attr, * Aggregating node and the AS_PATH is to be * constructed "as in all other cases" */ - if ( attre->aggregator_as != BGP_AS_TRANS ) + if (attre->aggregator_as != BGP_AS_TRANS) { /* ignore */ if ( BGP_DEBUG(as4, AS4)) @@ -1146,24 +1228,27 @@ bgp_attr_munge_as4_attrs (struct peer *peer, struct attr *attr, } /* need to reconcile NEW_AS_PATH and AS_PATH */ - if ( !ignore_as4_path && (attr->flag & ( ATTR_FLAG_BIT( BGP_ATTR_AS4_PATH))) ) + if (!ignore_as4_path && (attr->flag & (ATTR_FLAG_BIT( BGP_ATTR_AS4_PATH)))) { newpath = aspath_reconcile_as4 (attr->aspath, as4_path); aspath_unintern (&attr->aspath); attr->aspath = aspath_intern (newpath); } - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* Community attribute. */ -static int +static bgp_attr_parse_ret_t bgp_attr_community (struct peer *peer, bgp_size_t length, - struct attr *attr, u_char flag) + struct attr *attr, u_char flag, u_char *startp) { + bgp_size_t total + = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); + if (length == 0) { attr->community = NULL; - return 0; + return BGP_ATTR_PARSE_PROCEED; } attr->community = @@ -1173,15 +1258,17 @@ bgp_attr_community (struct peer *peer, bgp_size_t length, stream_forward_getp (peer->ibuf, length); if (!attr->community) - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_COMMUNITIES, flag, + BGP_NOTIFY_UPDATE_OPT_ATTR_ERR, + startp, total); attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_COMMUNITIES); - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* Originator ID attribute. */ -static int +static bgp_attr_parse_ret_t bgp_attr_originator_id (struct peer *peer, bgp_size_t length, struct attr *attr, u_char flag) { @@ -1189,10 +1276,9 @@ bgp_attr_originator_id (struct peer *peer, bgp_size_t length, { zlog (peer->log, LOG_ERR, "Bad originator ID length %d", length); - bgp_notify_send (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_LENG_ERR); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_ORIGINATOR_ID, flag, + BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, + NULL, 0); } (bgp_attr_extra_get (attr))->originator_id.s_addr @@ -1200,11 +1286,11 @@ bgp_attr_originator_id (struct peer *peer, bgp_size_t length, attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_ORIGINATOR_ID); - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* Cluster list attribute. */ -static int +static bgp_attr_parse_ret_t bgp_attr_cluster_list (struct peer *peer, bgp_size_t length, struct attr *attr, u_char flag) { @@ -1213,20 +1299,20 @@ bgp_attr_cluster_list (struct peer *peer, bgp_size_t length, { zlog (peer->log, LOG_ERR, "Bad cluster list length %d", length); - bgp_notify_send (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_LENG_ERR); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_CLUSTER_LIST, flag, + BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, + NULL, 0); } (bgp_attr_extra_get (attr))->cluster = cluster_parse ((struct in_addr *)stream_pnt (peer->ibuf), length); - - stream_forward_getp (peer->ibuf, length);; + + /* XXX: Fix cluster_parse to use stream API and then remove this */ + stream_forward_getp (peer->ibuf, length); attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_CLUSTER_LIST); - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* Multiprotocol reachability information parse. */ @@ -1253,7 +1339,7 @@ bgp_mp_reach_parse (struct peer *peer, bgp_size_t length, struct attr *attr, { zlog_info ("%s: %s sent invalid length, %lu", __func__, peer->host, (unsigned long)length); - return -1; + return BGP_ATTR_PARSE_ERROR; } /* Load AFI, SAFI. */ @@ -1267,7 +1353,7 @@ bgp_mp_reach_parse (struct peer *peer, bgp_size_t length, struct attr *attr, { zlog_info ("%s: %s, MP nexthop length, %u, goes past end of attribute", __func__, peer->host, attre->mp_nexthop_len); - return -1; + return BGP_ATTR_PARSE_ERROR; } /* Nexthop length check. */ @@ -1315,14 +1401,14 @@ bgp_mp_reach_parse (struct peer *peer, bgp_size_t length, struct attr *attr, default: zlog_info ("%s: (%s) Wrong multiprotocol next hop length: %d", __func__, peer->host, attre->mp_nexthop_len); - return -1; + return BGP_ATTR_PARSE_ERROR; } if (!LEN_LEFT) { zlog_info ("%s: (%s) Failed to read SNPA and NLRI(s)", __func__, peer->host); - return -1; + return BGP_ATTR_PARSE_ERROR; } { @@ -1338,7 +1424,7 @@ bgp_mp_reach_parse (struct peer *peer, bgp_size_t length, struct attr *attr, { zlog_info ("%s: (%s) Failed to read NLRI", __func__, peer->host); - return -1; + return BGP_ATTR_PARSE_ERROR; } if (safi != BGP_SAFI_VPNV4) @@ -1348,7 +1434,7 @@ bgp_mp_reach_parse (struct peer *peer, bgp_size_t length, struct attr *attr, { zlog_info ("%s: (%s) NLRI doesn't pass sanity check", __func__, peer->host); - return -1; + return BGP_ATTR_PARSE_ERROR; } } @@ -1359,7 +1445,7 @@ bgp_mp_reach_parse (struct peer *peer, bgp_size_t length, struct attr *attr, stream_forward_getp (s, nlri_len); - return 0; + return BGP_ATTR_PARSE_PROCEED; #undef LEN_LEFT } @@ -1378,7 +1464,7 @@ bgp_mp_unreach_parse (struct peer *peer, bgp_size_t length, #define BGP_MP_UNREACH_MIN_SIZE 3 if ((length > STREAM_READABLE(s)) || (length < BGP_MP_UNREACH_MIN_SIZE)) - return -1; + return BGP_ATTR_PARSE_ERROR; afi = stream_getw (s); safi = stream_getc (s); @@ -1389,7 +1475,7 @@ bgp_mp_unreach_parse (struct peer *peer, bgp_size_t length, { ret = bgp_nlri_sanity_check (peer, afi, stream_pnt (s), withdraw_len); if (ret < 0) - return -1; + return BGP_ATTR_PARSE_ERROR; } mp_withdraw->afi = afi; @@ -1399,20 +1485,23 @@ bgp_mp_unreach_parse (struct peer *peer, bgp_size_t length, stream_forward_getp (s, withdraw_len); - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* Extended Community attribute. */ -static int +static bgp_attr_parse_ret_t bgp_attr_ext_communities (struct peer *peer, bgp_size_t length, - struct attr *attr, u_char flag) + struct attr *attr, u_char flag, u_char *startp) { + bgp_size_t total + = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); + if (length == 0) { if (attr->extra) attr->extra->ecommunity = NULL; /* Empty extcomm doesn't seem to be invalid per se */ - return 0; + return BGP_ATTR_PARSE_PROCEED; } (bgp_attr_extra_get (attr))->ecommunity = @@ -1421,15 +1510,17 @@ bgp_attr_ext_communities (struct peer *peer, bgp_size_t length, stream_forward_getp (peer->ibuf, length); if (!attr->extra->ecommunity) - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_EXT_COMMUNITIES, + flag, BGP_NOTIFY_UPDATE_OPT_ATTR_ERR, + startp, total); attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_EXT_COMMUNITIES); - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* BGP unknown attribute treatment. */ -static int +static bgp_attr_parse_ret_t bgp_attr_unknown (struct peer *peer, struct attr *attr, u_char flag, u_char type, bgp_size_t length, u_char *startp) { @@ -1455,20 +1546,17 @@ bgp_attr_unknown (struct peer *peer, struct attr *attr, u_char flag, then the Error Subcode is set to Unrecognized Well-known Attribute. The Data field contains the unrecognized attribute (type, length and value). */ - if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) + if (!CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) { - /* Adjust startp to do not include flag value. */ - bgp_notify_send_with_data (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_UNREC_ATTR, - startp, total); - return -1; + return bgp_attr_malformed (peer, type, flag, + BGP_NOTIFY_UPDATE_UNREC_ATTR, + startp, total); } /* Unrecognized non-transitive optional attributes must be quietly ignored and not passed along to other BGP peers. */ if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) - return 0; + return BGP_ATTR_PARSE_PROCEED; /* If a path with recognized transitive optional attribute is accepted and passed along to other BGP peers and the Partial bit @@ -1491,17 +1579,17 @@ bgp_attr_unknown (struct peer *peer, struct attr *attr, u_char flag, memcpy (transit->val + transit->length, startp, total); transit->length += total; - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* Read attribute of update packet. This function is called from bgp_update() in bgpd.c. */ -int +bgp_attr_parse_ret_t bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, struct bgp_nlri *mp_update, struct bgp_nlri *mp_withdraw) { int ret; - u_char flag; + u_char flag = 0; u_char type = 0; bgp_size_t length; u_char *startp, *endp; @@ -1518,7 +1606,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, /* End pointer of BGP attribute. */ endp = BGP_INPUT_PNT (peer) + size; - + /* Get attributes to the end of attribute length. */ while (BGP_INPUT_PNT (peer) < endp) { @@ -1534,7 +1622,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR); - return -1; + return BGP_ATTR_PARSE_ERROR; } /* Fetch attribute flag and type. */ @@ -1554,7 +1642,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR); - return -1; + return BGP_ATTR_PARSE_ERROR; } /* Check extended attribue length bit. */ @@ -1576,7 +1664,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_MAL_ATTR); - return -1; + return BGP_ATTR_PARSE_ERROR; } /* Set type to bitmap to check duplicate attribute. `type' is @@ -1594,7 +1682,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR); - return -1; + return BGP_ATTR_PARSE_ERROR; } /* OK check attribute and store it's value. */ @@ -1607,7 +1695,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, ret = bgp_attr_aspath (peer, length, attr, flag, startp); break; case BGP_ATTR_AS4_PATH: - ret = bgp_attr_as4_path (peer, length, attr, &as4_path ); + ret = bgp_attr_as4_path (peer, length, attr, flag, startp, &as4_path); break; case BGP_ATTR_NEXT_HOP: ret = bgp_attr_nexthop (peer, length, attr, flag, startp); @@ -1625,10 +1713,12 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, ret = bgp_attr_aggregator (peer, length, attr, flag); break; case BGP_ATTR_AS4_AGGREGATOR: - ret = bgp_attr_as4_aggregator (peer, length, attr, &as4_aggregator, &as4_aggregator_addr); + ret = bgp_attr_as4_aggregator (peer, length, attr, flag, + &as4_aggregator, + &as4_aggregator_addr); break; case BGP_ATTR_COMMUNITIES: - ret = bgp_attr_community (peer, length, attr, flag); + ret = bgp_attr_community (peer, length, attr, flag, startp); break; case BGP_ATTR_ORIGINATOR_ID: ret = bgp_attr_originator_id (peer, length, attr, flag); @@ -1643,26 +1733,39 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, ret = bgp_mp_unreach_parse (peer, length, mp_withdraw); break; case BGP_ATTR_EXT_COMMUNITIES: - ret = bgp_attr_ext_communities (peer, length, attr, flag); + ret = bgp_attr_ext_communities (peer, length, attr, flag, startp); break; default: ret = bgp_attr_unknown (peer, attr, flag, type, length, startp); break; } - - /* If error occured immediately return to the caller. */ - if (ret < 0) + + /* If hard error occured immediately return to the caller. */ + if (ret == BGP_ATTR_PARSE_ERROR) { zlog (peer->log, LOG_WARNING, "%s: Attribute %s, parse error", peer->host, LOOKUP (attr_str, type)); - bgp_notify_send (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_MAL_ATTR); - return ret; + bgp_notify_send (peer, + BGP_NOTIFY_UPDATE_ERR, + BGP_NOTIFY_UPDATE_MAL_ATTR); + if (as4_path) + aspath_unintern (&as4_path); + return ret; } - + if (ret == BGP_ATTR_PARSE_WITHDRAW) + { + + zlog (peer->log, LOG_WARNING, + "%s: Attribute %s, parse error - treating as withdrawal", + peer->host, + LOOKUP (attr_str, type)); + if (as4_path) + aspath_unintern (&as4_path); + return ret; + } + /* Check the fetched length. */ if (BGP_INPUT_PNT (peer) != attr_endp) { @@ -1672,7 +1775,9 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR); - return -1; + if (as4_path) + aspath_unintern (&as4_path); + return BGP_ATTR_PARSE_ERROR; } } @@ -1685,7 +1790,9 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR); - return -1; + if (as4_path) + aspath_unintern (&as4_path); + return BGP_ATTR_PARSE_ERROR; } /* @@ -1699,16 +1806,20 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, * all attributes first, including these 32bit ones, and now, * afterwards, we look what and if something is to be done for as4. */ - if (bgp_attr_munge_as4_attrs (peer, attr, as4_path, + if (bgp_attr_munge_as4_attrs (peer, attr, flag, as4_path, as4_aggregator, &as4_aggregator_addr)) - return -1; + { + if (as4_path) + aspath_unintern (&as4_path); + return BGP_ATTR_PARSE_ERROR; + } /* At this stage, we have done all fiddling with as4, and the * resulting info is in attr->aggregator resp. attr->aspath * so we can chuck as4_aggregator and as4_path alltogether in * order to save memory */ - if ( as4_path ) + if (as4_path) { aspath_unintern (&as4_path); /* unintern - it is in the hash */ /* The flag that we got this is still there, but that does not @@ -1725,10 +1836,10 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, * Finally do the checks on the aspath we did not do yet * because we waited for a potentially synthesized aspath. */ - if ( attr->flag & ( ATTR_FLAG_BIT( BGP_ATTR_AS_PATH))) + if (attr->flag & (ATTR_FLAG_BIT(BGP_ATTR_AS_PATH))) { - ret = bgp_attr_aspath_check( peer, attr ); - if ( ret < 0 ) + ret = bgp_attr_aspath_check (peer, attr, flag); + if (ret != BGP_ATTR_PARSE_PROCEED) return ret; } @@ -1736,7 +1847,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, if (attr->extra && attr->extra->transit) attr->extra->transit = transit_intern (attr->extra->transit); - return 0; + return BGP_ATTR_PARSE_PROCEED; } /* Well-known attribute check. */ @@ -1767,9 +1878,9 @@ bgp_attr_check (struct peer *peer, struct attr *attr) BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_MISS_ATTR, &type, 1); - return -1; + return BGP_ATTR_PARSE_ERROR; } - return 0; + return BGP_ATTR_PARSE_PROCEED; } int stream_put_prefix (struct stream *, struct prefix *); -- cgit v1.2.1 From d68ab1009f42c5ed62f7c6b0b5700f5f0469a35b Mon Sep 17 00:00:00 2001 From: heasley Date: Tue, 12 Jul 2011 20:09:18 +0400 Subject: bgpd: consistent log msg format (BZ#565) --- bgpd/bgp_attr.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index d43c104f..0af240e2 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -1615,7 +1615,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, { /* XXX warning: long int format, int arg (arg 5) */ zlog (peer->log, LOG_WARNING, - "%s error BGP attribute length %lu is smaller than min len", + "%s: error BGP attribute length %lu is smaller than min len", peer->host, (unsigned long) (endp - STREAM_PNT (BGP_INPUT (peer)))); @@ -1635,7 +1635,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, && ((endp - startp) < (BGP_ATTR_MIN_LEN + 1))) { zlog (peer->log, LOG_WARNING, - "%s Extended length set, but just %lu bytes of attr header", + "%s: Extended length set, but just %lu bytes of attr header", peer->host, (unsigned long) (endp - STREAM_PNT (BGP_INPUT (peer)))); @@ -1658,7 +1658,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, if (CHECK_BITMAP (seen, type)) { zlog (peer->log, LOG_WARNING, - "%s error BGP attribute type %d appears twice in a message", + "%s: error BGP attribute type %d appears twice in a message", peer->host, type); bgp_notify_send (peer, @@ -1678,7 +1678,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, if (attr_endp > endp) { zlog (peer->log, LOG_WARNING, - "%s BGP type %d length %d is too large, attribute total length is %d. attr_endp is %p. endp is %p", peer->host, type, length, size, attr_endp, endp); + "%s: BGP type %d length %d is too large, attribute total length is %d. attr_endp is %p. endp is %p", peer->host, type, length, size, attr_endp, endp); bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR); @@ -1785,7 +1785,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, if (BGP_INPUT_PNT (peer) != endp) { zlog (peer->log, LOG_WARNING, - "%s BGP attribute %s, length mismatch", + "%s: BGP attribute %s, length mismatch", peer->host, LOOKUP (attr_str, type)); bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, -- cgit v1.2.1 From 0ea968d21f194b3960a73aa47a5b06f160632907 Mon Sep 17 00:00:00 2001 From: Denis Ovsienko Date: Mon, 19 Sep 2011 16:30:47 +0400 Subject: bgpd: check LOCAL_PREF attribute flags (BZ#674) * bgp_attr.c * bgp_attr_local_pref(): accept extra argument, add checks for "optional" and "transitive" bits, log each error condition independently * bgp_attr_parse(): provide extra argument --- bgpd/bgp_attr.c | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index 0af240e2..337ddb7c 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -1025,8 +1025,33 @@ bgp_attr_med (struct peer *peer, bgp_size_t length, /* Local preference attribute. */ static bgp_attr_parse_ret_t bgp_attr_local_pref (struct peer *peer, bgp_size_t length, - struct attr *attr, u_char flag) + struct attr *attr, u_char flag, u_char *startp) { + bgp_size_t total; + + total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); + /* Flag checks. */ + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) + { + zlog (peer->log, LOG_ERR, + "LOCAL_PREF attribute must be flagged as \"well-known\" (%u)", flag); + bgp_notify_send_with_data (peer, + BGP_NOTIFY_UPDATE_ERR, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); + return -1; + } + if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) + { + zlog (peer->log, LOG_ERR, + "LOCAL_PREF attribute must be flagged as \"transitive\" (%u)", flag); + bgp_notify_send_with_data (peer, + BGP_NOTIFY_UPDATE_ERR, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); + return -1; + } + /* If it is contained in an UPDATE message that is received from an external peer, then this attribute MUST be ignored by the receiving speaker. */ @@ -1704,7 +1729,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, ret = bgp_attr_med (peer, length, attr, flag, startp); break; case BGP_ATTR_LOCAL_PREF: - ret = bgp_attr_local_pref (peer, length, attr, flag); + ret = bgp_attr_local_pref (peer, length, attr, flag, startp); break; case BGP_ATTR_ATOMIC_AGGREGATE: ret = bgp_attr_atomic (peer, length, attr, flag); -- cgit v1.2.1 From 2cfadf09a810e3b660a47a1eb898d0d7c8813c08 Mon Sep 17 00:00:00 2001 From: Denis Ovsienko Date: Tue, 20 Sep 2011 10:54:25 +0400 Subject: bgpd: check MULTI_EXIT_DISC attr flags (BZ#677) * bgp_attr.c * bgp_attr_med(): add checks for "optional", "transitive" and "partial" bits, log each error condition independently --- bgpd/bgp_attr.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index 337ddb7c..a9866994 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -1004,6 +1004,38 @@ bgp_attr_med (struct peer *peer, bgp_size_t length, total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); + /* Flag checks. */ + if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) + { + zlog (peer->log, LOG_ERR, + "MULTI_EXIT_DISC attribute must be flagged as \"optional\" (%u)", flag); + bgp_notify_send_with_data (peer, + BGP_NOTIFY_UPDATE_ERR, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); + return -1; + } + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) + { + zlog (peer->log, LOG_ERR, + "MULTI_EXIT_DISC attribute must not be flagged as \"transitive\" (%u)", flag); + bgp_notify_send_with_data (peer, + BGP_NOTIFY_UPDATE_ERR, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); + return -1; + } + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) + { + zlog (peer->log, LOG_ERR, + "MULTI_EXIT_DISC attribute must not be flagged as \"partial\" (%u)", flag); + bgp_notify_send_with_data (peer, + BGP_NOTIFY_UPDATE_ERR, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); + return -1; + } + /* Length check. */ if (length != 4) { -- cgit v1.2.1 From 9eba2ada189597fcb39c806bbbed414fecf20623 Mon Sep 17 00:00:00 2001 From: Denis Ovsienko Date: Tue, 20 Sep 2011 14:43:50 +0400 Subject: bgpd: check ATOMIC_AGGREGATE attr flags (BZ#678) * bgp_attr.c * bgp_attr_atomic(): accept extra argument, add checks for "optional", "transitive" and "partial" bits, log each error condition independently * bgp_attr_parse(): provide extra argument --- bgpd/bgp_attr.c | 40 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index a9866994..015ae59c 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -1107,8 +1107,44 @@ bgp_attr_local_pref (struct peer *peer, bgp_size_t length, /* Atomic aggregate. */ static int bgp_attr_atomic (struct peer *peer, bgp_size_t length, - struct attr *attr, u_char flag) + struct attr *attr, u_char flag, u_char *startp) { + bgp_size_t total; + + total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); + /* Flag checks. */ + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) + { + zlog (peer->log, LOG_ERR, + "ATOMIC_AGGREGATE attribute must not be flagged as \"optional\" (%u)", flag); + bgp_notify_send_with_data (peer, + BGP_NOTIFY_UPDATE_ERR, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); + return -1; + } + if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) + { + zlog (peer->log, LOG_ERR, + "ATOMIC_AGGREGATE attribute must be flagged as \"transitive\" (%u)", flag); + bgp_notify_send_with_data (peer, + BGP_NOTIFY_UPDATE_ERR, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); + return -1; + } + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) + { + zlog (peer->log, LOG_ERR, + "ATOMIC_AGGREGATE attribute must not be flagged as \"partial\" (%u)", flag); + bgp_notify_send_with_data (peer, + BGP_NOTIFY_UPDATE_ERR, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); + return -1; + } + + /* Length check. */ if (length != 0) { zlog (peer->log, LOG_ERR, "Bad atomic aggregate length %d", length); @@ -1764,7 +1800,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, ret = bgp_attr_local_pref (peer, length, attr, flag, startp); break; case BGP_ATTR_ATOMIC_AGGREGATE: - ret = bgp_attr_atomic (peer, length, attr, flag); + ret = bgp_attr_atomic (peer, length, attr, flag, startp); break; case BGP_ATTR_AGGREGATOR: ret = bgp_attr_aggregator (peer, length, attr, flag); -- 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_attr.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index 015ae59c..d1ba0dac 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -1520,7 +1520,7 @@ bgp_mp_reach_parse (struct peer *peer, bgp_size_t length, struct attr *attr, return BGP_ATTR_PARSE_ERROR; } - if (safi != BGP_SAFI_VPNV4) + if (safi != SAFI_MPLS_LABELED_VPN) { ret = bgp_nlri_sanity_check (peer, afi, stream_pnt (s), nlri_len); if (ret < 0) @@ -1564,7 +1564,7 @@ bgp_mp_unreach_parse (struct peer *peer, bgp_size_t length, withdraw_len = length - BGP_MP_UNREACH_MIN_SIZE; - if (safi != BGP_SAFI_VPNV4) + if (safi != SAFI_MPLS_LABELED_VPN) { ret = bgp_nlri_sanity_check (peer, afi, stream_pnt (s), withdraw_len); if (ret < 0) @@ -2269,7 +2269,7 @@ bgp_packet_attribute (struct bgp *bgp, struct peer *peer, sizep = stream_get_endp (s); stream_putc (s, 0); /* Length of this attribute. */ stream_putw (s, AFI_IP); /* AFI */ - stream_putc (s, BGP_SAFI_VPNV4); /* SAFI */ + stream_putc (s, SAFI_MPLS_LABELED_VPN); /* SAFI */ stream_putc (s, 12); stream_putl (s, 0); @@ -2432,7 +2432,7 @@ bgp_packet_withdraw (struct peer *peer, struct stream *s, struct prefix *p, if (safi == SAFI_MPLS_VPN) { /* SAFI */ - stream_putc (s, BGP_SAFI_VPNV4); + stream_putc (s, SAFI_MPLS_LABELED_VPN); /* prefix. */ stream_putc (s, p->prefixlen + 88); -- cgit v1.2.1 From bc3443ebf032b5fcc9e0ccb94641e4e899cd17d8 Mon Sep 17 00:00:00 2001 From: Denis Ovsienko Date: Thu, 22 Sep 2011 12:48:14 +0400 Subject: bgpd: improve NEXT_HOP attribute checks (BZ#680) * lib/prefix.h * IPV4_CLASS_DE(): new helper macro * bgp_attr.c * bgp_attr_nexthop(): add check for "partial" bit, refresh flag error reporting, explain meaning of RFC4271 section 6.3 and implement it Conflicts: bgpd/bgp_attr.c --- bgpd/bgp_attr.c | 45 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 39 insertions(+), 6 deletions(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index d1ba0dac..6905ab52 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -964,15 +964,31 @@ bgp_attr_nexthop (struct peer *peer, bgp_size_t length, struct attr *attr, u_char flag, u_char *startp) { bgp_size_t total; + in_addr_t nexthop_h, nexthop_n; total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); - /* Flag check. */ - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL) - || ! CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) + /* Flags check. */ + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) { - zlog (peer->log, LOG_ERR, - "Origin attribute flag isn't transitive %d", flag); + zlog (peer->log, LOG_ERR, + "NEXT_HOP attribute must not be flagged as \"optional\" (%u)", flag); + return bgp_attr_malformed (peer, BGP_ATTR_NEXT_HOP, flag, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); + } + if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) + { + zlog (peer->log, LOG_ERR, + "NEXT_HOP attribute must be flagged as \"transitive\" (%u)", flag); + return bgp_attr_malformed (peer, BGP_ATTR_NEXT_HOP, flag, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); + } + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) + { + zlog (peer->log, LOG_ERR, + "NEXT_HOP attribute must not be flagged as \"partial\" (%u)", flag); return bgp_attr_malformed (peer, BGP_ATTR_NEXT_HOP, flag, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); @@ -989,7 +1005,24 @@ bgp_attr_nexthop (struct peer *peer, bgp_size_t length, startp, total); } - attr->nexthop.s_addr = stream_get_ipv4 (peer->ibuf); + /* According to section 6.3 of RFC4271, syntactically incorrect NEXT_HOP + attribute must result in a NOTIFICATION message (this is implemented below). + At the same time, semantically incorrect NEXT_HOP is more likely to be just + logged locally (this is implemented somewhere else). The UPDATE message + gets ignored in any of these cases. */ + nexthop_n = stream_get_ipv4 (peer->ibuf); + nexthop_h = ntohl (nexthop_n); + if (IPV4_NET0 (nexthop_h) || IPV4_NET127 (nexthop_h) || IPV4_CLASS_DE (nexthop_h)) + { + char buf[INET_ADDRSTRLEN]; + inet_ntop (AF_INET, &nexthop_h, buf, INET_ADDRSTRLEN); + zlog (peer->log, LOG_ERR, "Martian nexthop %s", buf); + return bgp_attr_malformed (peer, BGP_ATTR_NEXT_HOP, flag, + BGP_NOTIFY_UPDATE_INVAL_NEXT_HOP, + startp, total); + } + + attr->nexthop.s_addr = nexthop_n; attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_NEXT_HOP); return BGP_ATTR_PARSE_PROCEED; -- cgit v1.2.1 From 214bcaa13e092d9fff8f233e62ba28ca7eefbc43 Mon Sep 17 00:00:00 2001 From: Denis Ovsienko Date: Sat, 24 Sep 2011 13:20:43 +0400 Subject: bgpd: add missing "partial" flag checks (BZ#676) ORIGIN handling function used to have "partial" bit check and recent commits added it for NEXT_HOP, MULTI_EXIT_DISC and ATOMIC_AGGREGATE cases. This commit adds "partial" check for AS_PATH and LOCAL_PREF cases, which should leave attributes 1 through 6 inclusive completely covered with attribute flags checks. * bgp_attr.c * bgp_attr_origin(): use bit-by-bit checks for better diagnostics * bgp_attr_aspath(): add flag check * bgp_attr_local_pref(): idem Conflicts: bgpd/bgp_attr.c --- bgpd/bgp_attr.c | 41 ++++++++++++++++++++++++++++++++++++++--- 1 file changed, 38 insertions(+), 3 deletions(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index 6905ab52..aa4ce8f1 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -773,14 +773,30 @@ bgp_attr_origin (struct peer *peer, bgp_size_t length, with the Attribute Type Code, then the Error Subcode is set to Attribute Flags Error. The Data field contains the erroneous attribute (type, length and value). */ - if (flag != BGP_ATTR_FLAG_TRANS) + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) { - zlog (peer->log, LOG_ERR, - "Origin attribute flag isn't transitive %d", flag); + zlog (peer->log, LOG_ERR, + "ORIGIN attribute must not be flagged as \"optional\" (%u)", flag); return bgp_attr_malformed (peer, BGP_ATTR_ORIGIN, flag, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); } + if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) + { + zlog (peer->log, LOG_ERR, + "ORIGIN attribute must be flagged as \"transitive\" (%u)", flag); + return bgp_attr_malformed (peer, BGP_ATTR_ORIGIN, flag, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); + } + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) + { + zlog (peer->log, LOG_ERR, + "ORIGIN attribute must not be flagged as \"partial\" (%u)", flag); + return bgp_attr_malformed (peer, BGP_ATTR_ORIGIN, flag, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); + } /* If any recognized attribute has Attribute Length that conflicts with the expected length (based on the attribute type code), then @@ -830,6 +846,15 @@ bgp_attr_aspath (struct peer *peer, bgp_size_t length, total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); /* Flag check. */ + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) + { + zlog (peer->log, LOG_ERR, + "AS_PATH attribute must not be flagged as \"partial\" (%u)", flag); + return bgp_attr_malformed (peer, BGP_ATTR_ORIGIN, flag, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); + } + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL) || ! CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) { @@ -1116,6 +1141,16 @@ bgp_attr_local_pref (struct peer *peer, bgp_size_t length, startp, total); return -1; } + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) + { + zlog (peer->log, LOG_ERR, + "LOCAL_PREF attribute must not be flagged as \"partial\" (%u)", flag); + bgp_notify_send_with_data (peer, + BGP_NOTIFY_UPDATE_ERR, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); + return -1; + } /* If it is contained in an UPDATE message that is received from an external peer, then this attribute MUST be ignored by the -- cgit v1.2.1 From 2d42e68aa032ed2f11471aee444935918d35c8bb Mon Sep 17 00:00:00 2001 From: Denis Ovsienko Date: Tue, 27 Sep 2011 15:35:39 +0400 Subject: bgpd: ignore 4 bits of attribute flags byte --- bgpd/bgp_attr.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index aa4ce8f1..45a17fac 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -1788,7 +1788,10 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, /* Fetch attribute flag and type. */ startp = BGP_INPUT_PNT (peer); - flag = stream_getc (BGP_INPUT (peer)); + /* "The lower-order four bits of the Attribute Flags octet are + unused. They MUST be zero when sent and MUST be ignored when + received." */ + flag = 0xF0 & stream_getc (BGP_INPUT (peer)); type = stream_getc (BGP_INPUT (peer)); /* Check whether Extended-Length applies and is in bounds */ -- cgit v1.2.1 From b84b62dfb6ee9daf46c9e4c2c372b179f33be44c Mon Sep 17 00:00:00 2001 From: Denis Ovsienko Date: Tue, 27 Sep 2011 15:47:25 +0400 Subject: bgpd: improve attr flags checks Do not check each of the Optional/Transitive/Partial attribute flag bits, when their only valid combination is known in advance, but still perform bit-deep error message logging. This change assumes unused (low-order) 4 bits of the flag octet cleared. * bgp_attr.c * bgp_attr_origin(): rewrite check * bgp_attr_nexthop(): idem * bgp_attr_med(): idem * bgp_attr_local_pref(): idem * bgp_attr_atomic(): idem Conflicts: bgpd/bgp_attr.c --- bgpd/bgp_attr.c | 191 ++++++++++++++++---------------------------------------- 1 file changed, 53 insertions(+), 138 deletions(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index 45a17fac..daf14bd4 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -773,30 +773,16 @@ bgp_attr_origin (struct peer *peer, bgp_size_t length, with the Attribute Type Code, then the Error Subcode is set to Attribute Flags Error. The Data field contains the erroneous attribute (type, length and value). */ - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) - { - zlog (peer->log, LOG_ERR, - "ORIGIN attribute must not be flagged as \"optional\" (%u)", flag); - return bgp_attr_malformed (peer, BGP_ATTR_ORIGIN, flag, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - } - if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) - { - zlog (peer->log, LOG_ERR, - "ORIGIN attribute must be flagged as \"transitive\" (%u)", flag); - return bgp_attr_malformed (peer, BGP_ATTR_ORIGIN, flag, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - } - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) - { - zlog (peer->log, LOG_ERR, - "ORIGIN attribute must not be flagged as \"partial\" (%u)", flag); - return bgp_attr_malformed (peer, BGP_ATTR_ORIGIN, flag, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - } + if (flag != BGP_ATTR_FLAG_TRANS) + { + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) + zlog (peer->log, LOG_ERR, "ORIGIN attribute must not be flagged as \"optional\" (%u)", flag); + if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) + zlog (peer->log, LOG_ERR, "ORIGIN attribute must be flagged as \"transitive\" (%u)", flag); + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) + zlog (peer->log, LOG_ERR, "ORIGIN attribute must not be flagged as \"partial\" (%u)", flag); + return bgp_attr_malformed (peer, BGP_ATTR_ORIGIN, flag, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); + } /* If any recognized attribute has Attribute Length that conflicts with the expected length (based on the attribute type code), then @@ -994,30 +980,16 @@ bgp_attr_nexthop (struct peer *peer, bgp_size_t length, total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); /* Flags check. */ - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) - { - zlog (peer->log, LOG_ERR, - "NEXT_HOP attribute must not be flagged as \"optional\" (%u)", flag); - return bgp_attr_malformed (peer, BGP_ATTR_NEXT_HOP, flag, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - } - if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) - { - zlog (peer->log, LOG_ERR, - "NEXT_HOP attribute must be flagged as \"transitive\" (%u)", flag); - return bgp_attr_malformed (peer, BGP_ATTR_NEXT_HOP, flag, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - } - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) - { - zlog (peer->log, LOG_ERR, - "NEXT_HOP attribute must not be flagged as \"partial\" (%u)", flag); - return bgp_attr_malformed (peer, BGP_ATTR_NEXT_HOP, flag, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - } + if (flag != BGP_ATTR_FLAG_TRANS) + { + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) + zlog (peer->log, LOG_ERR, "NEXT_HOP attribute must not be flagged as \"optional\" (%u)", flag); + if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) + zlog (peer->log, LOG_ERR, "NEXT_HOP attribute must be flagged as \"transitive\" (%u)", flag); + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) + zlog (peer->log, LOG_ERR, "NEXT_HOP attribute must not be flagged as \"partial\" (%u)", flag); + return bgp_attr_malformed (peer, BGP_ATTR_NEXT_HOP, flag, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); + } /* Check nexthop attribute length. */ if (length != 4) @@ -1063,36 +1035,17 @@ bgp_attr_med (struct peer *peer, bgp_size_t length, total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); /* Flag checks. */ - if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) - { - zlog (peer->log, LOG_ERR, - "MULTI_EXIT_DISC attribute must be flagged as \"optional\" (%u)", flag); - bgp_notify_send_with_data (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - return -1; - } - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) - { - zlog (peer->log, LOG_ERR, - "MULTI_EXIT_DISC attribute must not be flagged as \"transitive\" (%u)", flag); - bgp_notify_send_with_data (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - return -1; - } - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) - { - zlog (peer->log, LOG_ERR, - "MULTI_EXIT_DISC attribute must not be flagged as \"partial\" (%u)", flag); - bgp_notify_send_with_data (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - return -1; - } + if (flag != BGP_ATTR_FLAG_OPTIONAL) + { + if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) + zlog (peer->log, LOG_ERR, "MULTI_EXIT_DISC attribute must be flagged as \"optional\" (%u)", flag); + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) + zlog (peer->log, LOG_ERR, "MULTI_EXIT_DISC attribute must not be flagged as \"transitive\" (%u)", flag); + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) + zlog (peer->log, LOG_ERR, "MULTI_EXIT_DISC attribute must not be flagged as \"partial\" (%u)", flag); + bgp_notify_send_with_data (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); + return -1; + } /* Length check. */ if (length != 4) @@ -1121,36 +1074,17 @@ bgp_attr_local_pref (struct peer *peer, bgp_size_t length, total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); /* Flag checks. */ - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) - { - zlog (peer->log, LOG_ERR, - "LOCAL_PREF attribute must be flagged as \"well-known\" (%u)", flag); - bgp_notify_send_with_data (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - return -1; - } - if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) - { - zlog (peer->log, LOG_ERR, - "LOCAL_PREF attribute must be flagged as \"transitive\" (%u)", flag); - bgp_notify_send_with_data (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - return -1; - } - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) - { - zlog (peer->log, LOG_ERR, - "LOCAL_PREF attribute must not be flagged as \"partial\" (%u)", flag); - bgp_notify_send_with_data (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - return -1; - } + if (flag != BGP_ATTR_FLAG_TRANS) + { + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) + zlog (peer->log, LOG_ERR, "LOCAL_PREF attribute must not be flagged as \"optional\" (%u)", flag); + if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) + zlog (peer->log, LOG_ERR, "LOCAL_PREF attribute must be flagged as \"transitive\" (%u)", flag); + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) + zlog (peer->log, LOG_ERR, "LOCAL_PREF attribute must not be flagged as \"partial\" (%u)", flag); + bgp_notify_send_with_data (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); + return -1; + } /* If it is contained in an UPDATE message that is received from an external peer, then this attribute MUST be ignored by the @@ -1181,36 +1115,17 @@ bgp_attr_atomic (struct peer *peer, bgp_size_t length, total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); /* Flag checks. */ - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) - { - zlog (peer->log, LOG_ERR, - "ATOMIC_AGGREGATE attribute must not be flagged as \"optional\" (%u)", flag); - bgp_notify_send_with_data (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - return -1; - } - if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) - { - zlog (peer->log, LOG_ERR, - "ATOMIC_AGGREGATE attribute must be flagged as \"transitive\" (%u)", flag); - bgp_notify_send_with_data (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - return -1; - } - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) - { - zlog (peer->log, LOG_ERR, - "ATOMIC_AGGREGATE attribute must not be flagged as \"partial\" (%u)", flag); - bgp_notify_send_with_data (peer, - BGP_NOTIFY_UPDATE_ERR, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - return -1; - } + if (flag != BGP_ATTR_FLAG_TRANS) + { + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) + zlog (peer->log, LOG_ERR, "ATOMIC_AGGREGATE attribute must not be flagged as \"optional\" (%u)", flag); + if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) + zlog (peer->log, LOG_ERR, "ATOMIC_AGGREGATE attribute must be flagged as \"transitive\" (%u)", flag); + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) + zlog (peer->log, LOG_ERR, "ATOMIC_AGGREGATE attribute must not be flagged as \"partial\" (%u)", flag); + bgp_notify_send_with_data (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); + return -1; + } /* Length check. */ if (length != 0) -- cgit v1.2.1 From a624cae2b210a0e81c80c473f86b73e2be169962 Mon Sep 17 00:00:00 2001 From: Denis Ovsienko Date: Sat, 8 Oct 2011 13:54:48 +0400 Subject: bgpd: improve attr length error handling (BZ#679) * bgp_attr.c * bgp_attr_parse(): provide extra argument to bgp_attr_aggregator() * bgp_attr_local_pref(): use bgp_notify_send_with_data() * bgp_attr_atomic(): idem * bgp_attr_aggregator(): idem Conflicts: bgpd/bgp_attr.c --- bgpd/bgp_attr.c | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index daf14bd4..5c3d153a 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -1085,6 +1085,16 @@ bgp_attr_local_pref (struct peer *peer, bgp_size_t length, bgp_notify_send_with_data (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); return -1; } + /* Length check. */ + if (length != 4) + { + zlog (peer->log, LOG_ERR, "LOCAL_PREF attribute length isn't 4 [%u]", length); + bgp_notify_send_with_data (peer, + BGP_NOTIFY_UPDATE_ERR, + BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, + startp, total); + return -1; + } /* If it is contained in an UPDATE message that is received from an external peer, then this attribute MUST be ignored by the @@ -1095,10 +1105,7 @@ bgp_attr_local_pref (struct peer *peer, bgp_size_t length, return BGP_ATTR_PARSE_PROCEED; } - if (length == 4) - attr->local_pref = stream_getl (peer->ibuf); - else - attr->local_pref = 0; + attr->local_pref = stream_getl (peer->ibuf); /* Set atomic aggregate flag. */ attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_LOCAL_PREF); @@ -1130,11 +1137,10 @@ bgp_attr_atomic (struct peer *peer, bgp_size_t length, /* Length check. */ if (length != 0) { - zlog (peer->log, LOG_ERR, "Bad atomic aggregate length %d", length); - + zlog (peer->log, LOG_ERR, "ATOMIC_AGGREGATE attribute length isn't 0 [%u]", length); return bgp_attr_malformed (peer, BGP_ATTR_ATOMIC_AGGREGATE, flag, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, - NULL, 0); + startp, total); } /* Set atomic aggregate flag. */ @@ -1146,22 +1152,23 @@ bgp_attr_atomic (struct peer *peer, bgp_size_t length, /* Aggregator attribute */ static int bgp_attr_aggregator (struct peer *peer, bgp_size_t length, - struct attr *attr, u_char flag) + struct attr *attr, u_char flag, u_char *startp) { int wantedlen = 6; struct attr_extra *attre = bgp_attr_extra_get (attr); + bgp_size_t total; + total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); /* peer with AS4 will send 4 Byte AS, peer without will send 2 Byte */ if (CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV)) wantedlen = 8; if (length != wantedlen) { - zlog (peer->log, LOG_ERR, "Aggregator length is not %d [%d]", wantedlen, length); - + zlog (peer->log, LOG_ERR, "AGGREGATOR attribute length isn't %u [%u]", wantedlen, length); return bgp_attr_malformed (peer, BGP_ATTR_AGGREGATOR, flag, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, - NULL, 0); + startp, total); } if ( CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV ) ) @@ -1789,7 +1796,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, ret = bgp_attr_atomic (peer, length, attr, flag, startp); break; case BGP_ATTR_AGGREGATOR: - ret = bgp_attr_aggregator (peer, length, attr, flag); + ret = bgp_attr_aggregator (peer, length, attr, flag, startp); break; case BGP_ATTR_AS4_AGGREGATOR: ret = bgp_attr_as4_aggregator (peer, length, attr, flag, -- cgit v1.2.1 From a5b228b3792937c93b589938a2545b9311b7938e Mon Sep 17 00:00:00 2001 From: Denis Ovsienko Date: Wed, 12 Oct 2011 13:54:21 +0400 Subject: bgpd: fix regression in improved attr flag checks Commit 2febf323411c1aed9d7694898f852ce2ef36a7e5 assumed every flag bit except optional/transitive/partial unset, which at times could not be true for "extended length" bit. * bgp_attr.c * bgp_attr_origin(): exclude BGP_ATTR_FLAG_EXTLEN from comparison * bgp_attr_nexthop(): idem * bgp_attr_med(): idem * bgp_attr_local_pref(): idem * bgp_attr_atomic(): idem --- bgpd/bgp_attr.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index 5c3d153a..cda522a0 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -773,7 +773,7 @@ bgp_attr_origin (struct peer *peer, bgp_size_t length, with the Attribute Type Code, then the Error Subcode is set to Attribute Flags Error. The Data field contains the erroneous attribute (type, length and value). */ - if (flag != BGP_ATTR_FLAG_TRANS) + if ((flag & ~BGP_ATTR_FLAG_EXTLEN) != BGP_ATTR_FLAG_TRANS) { if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) zlog (peer->log, LOG_ERR, "ORIGIN attribute must not be flagged as \"optional\" (%u)", flag); @@ -980,7 +980,7 @@ bgp_attr_nexthop (struct peer *peer, bgp_size_t length, total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); /* Flags check. */ - if (flag != BGP_ATTR_FLAG_TRANS) + if ((flag & ~BGP_ATTR_FLAG_EXTLEN) != BGP_ATTR_FLAG_TRANS) { if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) zlog (peer->log, LOG_ERR, "NEXT_HOP attribute must not be flagged as \"optional\" (%u)", flag); @@ -1035,7 +1035,7 @@ bgp_attr_med (struct peer *peer, bgp_size_t length, total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); /* Flag checks. */ - if (flag != BGP_ATTR_FLAG_OPTIONAL) + if ((flag & ~BGP_ATTR_FLAG_EXTLEN) != BGP_ATTR_FLAG_OPTIONAL) { if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) zlog (peer->log, LOG_ERR, "MULTI_EXIT_DISC attribute must be flagged as \"optional\" (%u)", flag); @@ -1074,7 +1074,7 @@ bgp_attr_local_pref (struct peer *peer, bgp_size_t length, total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); /* Flag checks. */ - if (flag != BGP_ATTR_FLAG_TRANS) + if ((flag & ~BGP_ATTR_FLAG_EXTLEN) != BGP_ATTR_FLAG_TRANS) { if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) zlog (peer->log, LOG_ERR, "LOCAL_PREF attribute must not be flagged as \"optional\" (%u)", flag); @@ -1122,7 +1122,7 @@ bgp_attr_atomic (struct peer *peer, bgp_size_t length, total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); /* Flag checks. */ - if (flag != BGP_ATTR_FLAG_TRANS) + if ((flag & ~BGP_ATTR_FLAG_EXTLEN) != BGP_ATTR_FLAG_TRANS) { if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) zlog (peer->log, LOG_ERR, "ATOMIC_AGGREGATE attribute must not be flagged as \"optional\" (%u)", flag); -- cgit v1.2.1 From d595b566bca1de8b026c469a31ae50f2aee11781 Mon Sep 17 00:00:00 2001 From: Denis Ovsienko Date: Fri, 30 Sep 2011 15:08:54 +0400 Subject: bgpd: check ORIGINATOR_ID attribute flags * bgp_attr.c * bgp_attr_originator_id(): accept extra argument, add checks for "optional", "transitive" and "partial" bits, log each error condition independently * bgp_attr_parse(): provide extra arguments --- bgpd/bgp_attr.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index cda522a0..a3db9f15 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -1353,8 +1353,24 @@ bgp_attr_community (struct peer *peer, bgp_size_t length, /* Originator ID attribute. */ static bgp_attr_parse_ret_t bgp_attr_originator_id (struct peer *peer, bgp_size_t length, - struct attr *attr, u_char flag) + struct attr *attr, u_char flag, u_char *startp) { + bgp_size_t total; + + total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); + /* Flag checks. */ + if (flag != BGP_ATTR_FLAG_OPTIONAL) + { + if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) + zlog (peer->log, LOG_ERR, "ORIGINATOR_ID attribute must be flagged as \"optional\" (%u)", flag); + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) + zlog (peer->log, LOG_ERR, "ORIGINATOR_ID attribute must not be flagged as \"transitive\" (%u)", flag); + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) + zlog (peer->log, LOG_ERR, "ORIGINATOR_ID attribute must not be flagged as \"partial\" (%u)", flag); + bgp_notify_send_with_data (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); + return -1; + } + /* Length check. */ if (length != 4) { zlog (peer->log, LOG_ERR, "Bad originator ID length %d", length); @@ -1807,7 +1823,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, ret = bgp_attr_community (peer, length, attr, flag, startp); break; case BGP_ATTR_ORIGINATOR_ID: - ret = bgp_attr_originator_id (peer, length, attr, flag); + ret = bgp_attr_originator_id (peer, length, attr, flag, startp); break; case BGP_ATTR_CLUSTER_LIST: ret = bgp_attr_cluster_list (peer, length, attr, flag); -- cgit v1.2.1 From 0b83044b91d7ea50824cfd803d77213b9ff38ed9 Mon Sep 17 00:00:00 2001 From: Denis Ovsienko Date: Fri, 30 Sep 2011 15:12:17 +0400 Subject: bgpd: check CLUSTER_LIST attribute flags * bgp_attr.c * bgp_attr_cluster_list(): accept extra argument, add checks for "optional", "transitive" and "partial" bits, log each error condition independently * bgp_attr_parse(): provide extra arguments --- bgpd/bgp_attr.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index a3db9f15..6dd14893 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -1391,8 +1391,23 @@ bgp_attr_originator_id (struct peer *peer, bgp_size_t length, /* Cluster list attribute. */ static bgp_attr_parse_ret_t bgp_attr_cluster_list (struct peer *peer, bgp_size_t length, - struct attr *attr, u_char flag) + struct attr *attr, u_char flag, u_char *startp) { + bgp_size_t total; + + total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); + /* Flag checks. */ + if (flag != BGP_ATTR_FLAG_OPTIONAL) + { + if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) + zlog (peer->log, LOG_ERR, "CLUSTER_LIST attribute must be flagged as \"optional\" (%u)", flag); + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) + zlog (peer->log, LOG_ERR, "CLUSTER_LIST attribute must not be flagged as \"transitive\" (%u)", flag); + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) + zlog (peer->log, LOG_ERR, "CLUSTER_LIST attribute must not be flagged as \"partial\" (%u)", flag); + bgp_notify_send_with_data (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); + return -1; + } /* Check length. */ if (length % 4) { @@ -1826,7 +1841,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, ret = bgp_attr_originator_id (peer, length, attr, flag, startp); break; case BGP_ATTR_CLUSTER_LIST: - ret = bgp_attr_cluster_list (peer, length, attr, flag); + ret = bgp_attr_cluster_list (peer, length, attr, flag, startp); break; case BGP_ATTR_MP_REACH_NLRI: ret = bgp_mp_reach_parse (peer, length, attr, mp_update); -- cgit v1.2.1 From f8627ff1ef7642c8ee4758b1f046ef0d166dfbec Mon Sep 17 00:00:00 2001 From: Denis Ovsienko Date: Mon, 10 Oct 2011 16:52:20 +0400 Subject: bgpd: fix spelling of CLUSTER_LIST --- bgpd/bgp_attr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index 6dd14893..8a85fc22 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -51,7 +51,7 @@ static const struct message attr_str [] = { BGP_ATTR_AGGREGATOR, "AGGREGATOR" }, { BGP_ATTR_COMMUNITIES, "COMMUNITY" }, { BGP_ATTR_ORIGINATOR_ID, "ORIGINATOR_ID" }, - { BGP_ATTR_CLUSTER_LIST, "CLUSTERLIST" }, + { BGP_ATTR_CLUSTER_LIST, "CLUSTER_LIST" }, { BGP_ATTR_DPA, "DPA" }, { BGP_ATTR_ADVERTISER, "ADVERTISER"} , { BGP_ATTR_RCID_PATH, "RCID_PATH" }, -- cgit v1.2.1 From 565b828dc00cafd477dd69ce15f0f551ece67710 Mon Sep 17 00:00:00 2001 From: Denis Ovsienko Date: Mon, 10 Oct 2011 21:08:33 +0400 Subject: bgpd: add flag checks for MP_(UN)REACH_NLRI * bgp_attr.[ch] * bgp_mp_reach_parse(): add extra arguments and a uniform flag check block * bgp_mp_unreach_parse(): idem * bgp_attr_parse(): provide extra arguments * bgp_mp_attr_test.c * parse_test(): justify respective calls --- bgpd/bgp_attr.c | 42 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 36 insertions(+), 6 deletions(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index 8a85fc22..c2e5fede 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -1431,8 +1431,8 @@ bgp_attr_cluster_list (struct peer *peer, bgp_size_t length, /* Multiprotocol reachability information parse. */ int -bgp_mp_reach_parse (struct peer *peer, bgp_size_t length, struct attr *attr, - struct bgp_nlri *mp_update) +bgp_mp_reach_parse (struct peer *peer, const bgp_size_t length, + struct attr *attr, const u_char flag, u_char *startp, struct bgp_nlri *mp_update) { afi_t afi; safi_t safi; @@ -1441,7 +1441,21 @@ bgp_mp_reach_parse (struct peer *peer, bgp_size_t length, struct attr *attr, int ret; struct stream *s; struct attr_extra *attre = bgp_attr_extra_get(attr); - + bgp_size_t total; + + total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); + /* Flag checks. */ + if (flag != BGP_ATTR_FLAG_OPTIONAL) + { + if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) + zlog (peer->log, LOG_ERR, "MP_REACH_NLRI attribute must be flagged as \"optional\" (%u)", flag); + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) + zlog (peer->log, LOG_ERR, "MP_REACH_NLRI attribute must not be flagged as \"transitive\" (%u)", flag); + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) + zlog (peer->log, LOG_ERR, "MP_REACH_NLRI attribute must not be flagged as \"partial\" (%u)", flag); + bgp_notify_send_with_data (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); + return -1; + } /* Set end of packet. */ s = BGP_INPUT(peer); start = stream_get_getp(s); @@ -1565,7 +1579,8 @@ bgp_mp_reach_parse (struct peer *peer, bgp_size_t length, struct attr *attr, /* Multiprotocol unreachable parse */ int -bgp_mp_unreach_parse (struct peer *peer, bgp_size_t length, +bgp_mp_unreach_parse (struct peer *peer, const bgp_size_t length, + const u_char flag, u_char *startp, struct bgp_nlri *mp_withdraw) { struct stream *s; @@ -1573,6 +1588,21 @@ bgp_mp_unreach_parse (struct peer *peer, bgp_size_t length, safi_t safi; u_int16_t withdraw_len; int ret; + bgp_size_t total; + + total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); + /* Flag checks. */ + if (flag != BGP_ATTR_FLAG_OPTIONAL) + { + if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) + zlog (peer->log, LOG_ERR, "MP_UNREACH_NLRI attribute must be flagged as \"optional\" (%u)", flag); + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) + zlog (peer->log, LOG_ERR, "MP_UNREACH_NLRI attribute must not be flagged as \"transitive\" (%u)", flag); + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) + zlog (peer->log, LOG_ERR, "MP_UNREACH_NLRI attribute must not be flagged as \"partial\" (%u)", flag); + bgp_notify_send_with_data (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); + return -1; + } s = peer->ibuf; @@ -1844,10 +1874,10 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, ret = bgp_attr_cluster_list (peer, length, attr, flag, startp); break; case BGP_ATTR_MP_REACH_NLRI: - ret = bgp_mp_reach_parse (peer, length, attr, mp_update); + ret = bgp_mp_reach_parse (peer, length, attr, flag, startp, mp_update); break; case BGP_ATTR_MP_UNREACH_NLRI: - ret = bgp_mp_unreach_parse (peer, length, mp_withdraw); + ret = bgp_mp_unreach_parse (peer, length, flag, startp, mp_withdraw); break; case BGP_ATTR_EXT_COMMUNITIES: ret = bgp_attr_ext_communities (peer, length, attr, flag, startp); -- cgit v1.2.1 From abc384f85b622471ef5ba6c86acc540856da05cf Mon Sep 17 00:00:00 2001 From: Denis Ovsienko Date: Tue, 18 Oct 2011 13:55:07 +0400 Subject: bgpd: use bgp_attr_malformed() Some of the recent attribute flags/length checks copied from QRE use bgp_notify_send_with_data() directly, but master branch assumes using bgp_attr_malformed(). * bgp_attr.c * bgp_attr_med() * bgp_attr_local_pref() * bgp_attr_atomic() * bgp_attr_originator_id() * bgp_attr_cluster_list() * bgp_mp_reach_parse() * bgp_mp_unreach_parse() --- bgpd/bgp_attr.c | 39 ++++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 17 deletions(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index c2e5fede..a60bb8e5 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -1043,8 +1043,9 @@ bgp_attr_med (struct peer *peer, bgp_size_t length, zlog (peer->log, LOG_ERR, "MULTI_EXIT_DISC attribute must not be flagged as \"transitive\" (%u)", flag); if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) zlog (peer->log, LOG_ERR, "MULTI_EXIT_DISC attribute must not be flagged as \"partial\" (%u)", flag); - bgp_notify_send_with_data (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_MULTI_EXIT_DISC, flag, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); } /* Length check. */ @@ -1082,18 +1083,17 @@ bgp_attr_local_pref (struct peer *peer, bgp_size_t length, zlog (peer->log, LOG_ERR, "LOCAL_PREF attribute must be flagged as \"transitive\" (%u)", flag); if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) zlog (peer->log, LOG_ERR, "LOCAL_PREF attribute must not be flagged as \"partial\" (%u)", flag); - bgp_notify_send_with_data (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_LOCAL_PREF, flag, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); } /* Length check. */ if (length != 4) { zlog (peer->log, LOG_ERR, "LOCAL_PREF attribute length isn't 4 [%u]", length); - bgp_notify_send_with_data (peer, - BGP_NOTIFY_UPDATE_ERR, + return bgp_attr_malformed (peer, BGP_ATTR_LOCAL_PREF, flag, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, startp, total); - return -1; } /* If it is contained in an UPDATE message that is received from an @@ -1130,8 +1130,9 @@ bgp_attr_atomic (struct peer *peer, bgp_size_t length, zlog (peer->log, LOG_ERR, "ATOMIC_AGGREGATE attribute must be flagged as \"transitive\" (%u)", flag); if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) zlog (peer->log, LOG_ERR, "ATOMIC_AGGREGATE attribute must not be flagged as \"partial\" (%u)", flag); - bgp_notify_send_with_data (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_ATOMIC_AGGREGATE, flag, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); } /* Length check. */ @@ -1367,8 +1368,9 @@ bgp_attr_originator_id (struct peer *peer, bgp_size_t length, zlog (peer->log, LOG_ERR, "ORIGINATOR_ID attribute must not be flagged as \"transitive\" (%u)", flag); if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) zlog (peer->log, LOG_ERR, "ORIGINATOR_ID attribute must not be flagged as \"partial\" (%u)", flag); - bgp_notify_send_with_data (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_ORIGINATOR_ID, flag, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); } /* Length check. */ if (length != 4) @@ -1405,8 +1407,9 @@ bgp_attr_cluster_list (struct peer *peer, bgp_size_t length, zlog (peer->log, LOG_ERR, "CLUSTER_LIST attribute must not be flagged as \"transitive\" (%u)", flag); if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) zlog (peer->log, LOG_ERR, "CLUSTER_LIST attribute must not be flagged as \"partial\" (%u)", flag); - bgp_notify_send_with_data (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_CLUSTER_LIST, flag, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); } /* Check length. */ if (length % 4) @@ -1453,8 +1456,9 @@ bgp_mp_reach_parse (struct peer *peer, const bgp_size_t length, zlog (peer->log, LOG_ERR, "MP_REACH_NLRI attribute must not be flagged as \"transitive\" (%u)", flag); if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) zlog (peer->log, LOG_ERR, "MP_REACH_NLRI attribute must not be flagged as \"partial\" (%u)", flag); - bgp_notify_send_with_data (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_MP_REACH_NLRI, flag, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); } /* Set end of packet. */ s = BGP_INPUT(peer); @@ -1600,8 +1604,9 @@ bgp_mp_unreach_parse (struct peer *peer, const bgp_size_t length, zlog (peer->log, LOG_ERR, "MP_UNREACH_NLRI attribute must not be flagged as \"transitive\" (%u)", flag); if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) zlog (peer->log, LOG_ERR, "MP_UNREACH_NLRI attribute must not be flagged as \"partial\" (%u)", flag); - bgp_notify_send_with_data (peer, BGP_NOTIFY_UPDATE_ERR, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); - return -1; + return bgp_attr_malformed (peer, BGP_ATTR_MP_UNREACH_NLRI, flag, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); } s = peer->ibuf; -- cgit v1.2.1 From bbb04bf3bb03be27fbf17b6b3ee903b168201cf0 Mon Sep 17 00:00:00 2001 From: Denis Ovsienko Date: Tue, 18 Oct 2011 14:20:04 +0400 Subject: bgpd: fix more regressions in attr flag checks Commit 05a4936b713b9882171d0f7fb20b8439df23939e fixed some of the attributes involved, but not all. This commit should do it. * bgp_attr.c * bgp_attr_originator_id() * bgp_attr_cluster_list() * bgp_mp_reach_parse() * bgp_mp_unreach_parse() --- bgpd/bgp_attr.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index a60bb8e5..d7eeec1f 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -1360,7 +1360,7 @@ bgp_attr_originator_id (struct peer *peer, bgp_size_t length, total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); /* Flag checks. */ - if (flag != BGP_ATTR_FLAG_OPTIONAL) + if ((flag & ~BGP_ATTR_FLAG_EXTLEN) != BGP_ATTR_FLAG_OPTIONAL) { if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) zlog (peer->log, LOG_ERR, "ORIGINATOR_ID attribute must be flagged as \"optional\" (%u)", flag); @@ -1399,7 +1399,7 @@ bgp_attr_cluster_list (struct peer *peer, bgp_size_t length, total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); /* Flag checks. */ - if (flag != BGP_ATTR_FLAG_OPTIONAL) + if ((flag & ~BGP_ATTR_FLAG_EXTLEN) != BGP_ATTR_FLAG_OPTIONAL) { if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) zlog (peer->log, LOG_ERR, "CLUSTER_LIST attribute must be flagged as \"optional\" (%u)", flag); @@ -1448,7 +1448,7 @@ bgp_mp_reach_parse (struct peer *peer, const bgp_size_t length, total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); /* Flag checks. */ - if (flag != BGP_ATTR_FLAG_OPTIONAL) + if ((flag & ~BGP_ATTR_FLAG_EXTLEN) != BGP_ATTR_FLAG_OPTIONAL) { if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) zlog (peer->log, LOG_ERR, "MP_REACH_NLRI attribute must be flagged as \"optional\" (%u)", flag); @@ -1596,7 +1596,7 @@ bgp_mp_unreach_parse (struct peer *peer, const bgp_size_t length, total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); /* Flag checks. */ - if (flag != BGP_ATTR_FLAG_OPTIONAL) + if ((flag & ~BGP_ATTR_FLAG_EXTLEN) != BGP_ATTR_FLAG_OPTIONAL) { if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) zlog (peer->log, LOG_ERR, "MP_UNREACH_NLRI attribute must be flagged as \"optional\" (%u)", flag); -- cgit v1.2.1 From b4cd2421518be7e18046e974e25b9bf4cf769bd2 Mon Sep 17 00:00:00 2001 From: Denis Ovsienko Date: Sat, 22 Oct 2011 22:32:26 +0400 Subject: bgpd: check AGGREGATOR attr flags (BZ#678) * bgp_attr.c * bgp_attr_aggregator(): check Optional/Transitive flag bits --- bgpd/bgp_attr.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index d7eeec1f..75b7c797 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -1160,6 +1160,23 @@ bgp_attr_aggregator (struct peer *peer, bgp_size_t length, bgp_size_t total; total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); + /* Flags check. */ + if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) + { + zlog (peer->log, LOG_ERR, + "AGGREGATOR attribute must be flagged as \"optional\" (%u)", flag); + return bgp_attr_malformed (peer, BGP_ATTR_AGGREGATOR, flag, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); + } + if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) + { + zlog (peer->log, LOG_ERR, + "AGGREGATOR attribute must be flagged as \"transitive\" (%u)", flag); + return bgp_attr_malformed (peer, BGP_ATTR_AGGREGATOR, flag, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); + } /* peer with AS4 will send 4 Byte AS, peer without will send 2 Byte */ if (CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV)) wantedlen = 8; -- cgit v1.2.1 From 267551879e57a5c590f9e12b3a7d6985b28b3dac Mon Sep 17 00:00:00 2001 From: Denis Ovsienko Date: Wed, 26 Oct 2011 19:34:30 +0400 Subject: bgpd: fix 2 more cases of length error reporting * bgp_attr.c (bgp_attr_originator_id, bgp_attr_cluster_list): provide required arguments to bgp_attr_malformed() --- bgpd/bgp_attr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index 75b7c797..29877eb7 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -1396,7 +1396,7 @@ bgp_attr_originator_id (struct peer *peer, bgp_size_t length, return bgp_attr_malformed (peer, BGP_ATTR_ORIGINATOR_ID, flag, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, - NULL, 0); + startp, total); } (bgp_attr_extra_get (attr))->originator_id.s_addr @@ -1435,7 +1435,7 @@ bgp_attr_cluster_list (struct peer *peer, bgp_size_t length, return bgp_attr_malformed (peer, BGP_ATTR_CLUSTER_LIST, flag, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, - NULL, 0); + startp, total); } (bgp_attr_extra_get (attr))->cluster -- cgit v1.2.1 From ce0af6ff5a4f200035ed4134da72a67f49a21dd6 Mon Sep 17 00:00:00 2001 From: "Oleg A. Arkhangelsky" Date: Sat, 3 Dec 2011 15:18:19 +0400 Subject: bgpd: fix memory leak for extra attributes this fixes commit b881c7074bb698aeb1b099175b325734fc6e44d2 --- bgpd/bgp_attr.c | 1 + 1 file changed, 1 insertion(+) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index 29877eb7..4e604b18 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -675,6 +675,7 @@ bgp_attr_unintern (struct attr **attr) } bgp_attr_unintern_sub (&tmp); + bgp_attr_extra_free (&tmp); } void -- cgit v1.2.1 From 9206f9ecd5526778bf449bbb12f056d681d3b040 Mon Sep 17 00:00:00 2001 From: Stephen Hemminger Date: Sun, 18 Dec 2011 19:43:40 +0400 Subject: fix set never used warnings (This patch was modified to leave calls to stream_getl() in place, they are necessary for the stream's internal pointer to advance to the correct position. -- Denis) Signed-off-by: Denis Ovsienko Fix gcc warnings about varables that are set but never used. * bgpd/bgp_attr.c * cluster_unintern(): ret * transit_unintern(): ret * bgp_attr_default_intern(): attre * bgp_mp_reach_parse(): rd_high, rd_low * bgpd/bgp_route.c * bgp_announce_check_rsclient(): bgp * bgpd/bgp_zebra.c * zebra_read_ipv4(): ifindex * zebra_read_ipv6(): ifindex * bgpd/bgpd.c * bgp_config_write_peer(): filter * lib/distribute.c * distribute_list_all(): dist * distribute_list(): dist * distribute_list_prefix_all(): dist * distribute_list_prefix(): dist * lib/if_rmap.c * if_rmap(): if_rmap * lib/vty.c * vty_accept(): vty * lib/zclient.c * zclient_read(): ret * zebra/irdp_interface.c * if_group(): zi * zebra/rt_netlink.c * kernel_read(): ret, sock --- bgpd/bgp_attr.c | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index 4e604b18..a2d4f4ba 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -175,14 +175,12 @@ cluster_intern (struct cluster_list *cluster) void cluster_unintern (struct cluster_list *cluster) { - struct cluster_list *ret; - if (cluster->refcnt) cluster->refcnt--; if (cluster->refcnt == 0) { - ret = hash_release (cluster_hash, cluster); + hash_release (cluster_hash, cluster); cluster_free (cluster); } } @@ -235,14 +233,12 @@ transit_intern (struct transit *transit) void transit_unintern (struct transit *transit) { - struct transit *ret; - if (transit->refcnt) transit->refcnt--; if (transit->refcnt == 0) { - ret = hash_release (transit_hash, transit); + hash_release (transit_hash, transit); transit_free (transit); } } @@ -552,10 +548,9 @@ bgp_attr_default_intern (u_char origin) { struct attr attr; struct attr *new; - struct attr_extra *attre; memset (&attr, 0, sizeof (struct attr)); - attre = bgp_attr_extra_get (&attr); + bgp_attr_extra_get (&attr); bgp_attr_default_set(&attr, origin); @@ -1516,14 +1511,9 @@ bgp_mp_reach_parse (struct peer *peer, const bgp_size_t length, memcpy(&attr->nexthop.s_addr, &attre->mp_nexthop_global_in, 4); break; case 12: - { - u_int32_t rd_high; - u_int32_t rd_low; - - rd_high = stream_getl (s); - rd_low = stream_getl (s); - stream_get (&attre->mp_nexthop_global_in, s, 4); - } + stream_getl (s); /* RD high */ + stream_getl (s); /* RD low */ + stream_get (&attre->mp_nexthop_global_in, s, 4); break; #ifdef HAVE_IPV6 case 16: -- cgit v1.2.1 From afcb767922509c4d998f1c567e350b9809c148ab Mon Sep 17 00:00:00 2001 From: Denis Ovsienko Date: Sun, 23 Oct 2011 22:32:44 +0400 Subject: bgpd: rewrite attr flag error logging * bgp_attr.c * attr_flag_str: new message list * bgp_attr_flags_diagnose(): new function, implements previously added error logging in a generic way * bgp_attr_origin(): use bgp_attr_flags_diagnose() * bgp_attr_nexthop(): ditto * bgp_attr_med(): ditto * bgp_attr_local_pref(): ditto * bgp_attr_atomic(): ditto * bgp_attr_originator_id(): ditto * bgp_attr_cluster_list(): ditto * bgp_mp_reach_parse(): ditto * bgp_mp_unreach_parse(): ditto --- bgpd/bgp_attr.c | 108 ++++++++++++++++++++++++++++---------------------------- 1 file changed, 54 insertions(+), 54 deletions(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index a2d4f4ba..b013c23f 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -63,6 +63,17 @@ static const struct message attr_str [] = { BGP_ATTR_AS_PATHLIMIT, "AS_PATHLIMIT" }, }; static const int attr_str_max = sizeof(attr_str)/sizeof(attr_str[0]); + +static const struct message attr_flag_str[] = +{ + { BGP_ATTR_FLAG_OPTIONAL, "Optional" }, + { BGP_ATTR_FLAG_TRANS, "Transitive" }, + { BGP_ATTR_FLAG_PARTIAL, "Partial" }, + /* bgp_attr_flags_diagnose() relies on this bit being last in this list */ + { BGP_ATTR_FLAG_EXTLEN, "Extended Length" }, +}; +static const size_t attr_flag_str_max = + sizeof (attr_flag_str) / sizeof (attr_flag_str[0]); static struct hash *cluster_hash; @@ -754,6 +765,40 @@ bgp_attr_malformed (struct peer *peer, u_char type, u_char flag, return BGP_ATTR_PARSE_ERROR; } +/* Find out what is wrong with the path attribute flag bits and log the error. + "Flag bits" here stand for Optional, Transitive and Partial, but not for + Extended Length. Checking O/T/P bits at once implies, that the attribute + being diagnosed is defined by RFC as either a "well-known" or an "optional, + non-transitive" attribute. */ +static void +bgp_attr_flags_diagnose +( + struct peer * peer, + const u_int8_t attr_code, + u_int8_t desired_flags, /* how RFC says it must be */ + u_int8_t real_flags /* on wire */ +) +{ + u_char seen = 0, i; + + desired_flags &= ~BGP_ATTR_FLAG_EXTLEN; + real_flags &= ~BGP_ATTR_FLAG_EXTLEN; + for (i = 0; i <= 2; i++) /* O,T,P, but not E */ + if + ( + CHECK_FLAG (desired_flags, attr_flag_str[i].key) != + CHECK_FLAG (real_flags, attr_flag_str[i].key) + ) + { + zlog (peer->log, LOG_ERR, "%s attribute must%s be flagged as \"%s\"", + LOOKUP (attr_str, attr_code), + CHECK_FLAG (desired_flags, attr_flag_str[i].key) ? "" : " not", + attr_flag_str[i].str); + seen = 1; + } + assert (seen); +} + /* Get origin attribute of the update message. */ static bgp_attr_parse_ret_t bgp_attr_origin (struct peer *peer, bgp_size_t length, @@ -771,12 +816,7 @@ bgp_attr_origin (struct peer *peer, bgp_size_t length, attribute (type, length and value). */ if ((flag & ~BGP_ATTR_FLAG_EXTLEN) != BGP_ATTR_FLAG_TRANS) { - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) - zlog (peer->log, LOG_ERR, "ORIGIN attribute must not be flagged as \"optional\" (%u)", flag); - if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) - zlog (peer->log, LOG_ERR, "ORIGIN attribute must be flagged as \"transitive\" (%u)", flag); - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) - zlog (peer->log, LOG_ERR, "ORIGIN attribute must not be flagged as \"partial\" (%u)", flag); + bgp_attr_flags_diagnose (peer, BGP_ATTR_ORIGIN, BGP_ATTR_FLAG_TRANS, flag); return bgp_attr_malformed (peer, BGP_ATTR_ORIGIN, flag, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); } @@ -978,12 +1018,7 @@ bgp_attr_nexthop (struct peer *peer, bgp_size_t length, /* Flags check. */ if ((flag & ~BGP_ATTR_FLAG_EXTLEN) != BGP_ATTR_FLAG_TRANS) { - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) - zlog (peer->log, LOG_ERR, "NEXT_HOP attribute must not be flagged as \"optional\" (%u)", flag); - if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) - zlog (peer->log, LOG_ERR, "NEXT_HOP attribute must be flagged as \"transitive\" (%u)", flag); - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) - zlog (peer->log, LOG_ERR, "NEXT_HOP attribute must not be flagged as \"partial\" (%u)", flag); + bgp_attr_flags_diagnose (peer, BGP_ATTR_NEXT_HOP, BGP_ATTR_FLAG_TRANS, flag); return bgp_attr_malformed (peer, BGP_ATTR_NEXT_HOP, flag, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); } @@ -1033,12 +1068,7 @@ bgp_attr_med (struct peer *peer, bgp_size_t length, /* Flag checks. */ if ((flag & ~BGP_ATTR_FLAG_EXTLEN) != BGP_ATTR_FLAG_OPTIONAL) { - if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) - zlog (peer->log, LOG_ERR, "MULTI_EXIT_DISC attribute must be flagged as \"optional\" (%u)", flag); - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) - zlog (peer->log, LOG_ERR, "MULTI_EXIT_DISC attribute must not be flagged as \"transitive\" (%u)", flag); - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) - zlog (peer->log, LOG_ERR, "MULTI_EXIT_DISC attribute must not be flagged as \"partial\" (%u)", flag); + bgp_attr_flags_diagnose (peer, BGP_ATTR_MULTI_EXIT_DISC, BGP_ATTR_FLAG_OPTIONAL, flag); return bgp_attr_malformed (peer, BGP_ATTR_MULTI_EXIT_DISC, flag, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); @@ -1073,12 +1103,7 @@ bgp_attr_local_pref (struct peer *peer, bgp_size_t length, /* Flag checks. */ if ((flag & ~BGP_ATTR_FLAG_EXTLEN) != BGP_ATTR_FLAG_TRANS) { - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) - zlog (peer->log, LOG_ERR, "LOCAL_PREF attribute must not be flagged as \"optional\" (%u)", flag); - if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) - zlog (peer->log, LOG_ERR, "LOCAL_PREF attribute must be flagged as \"transitive\" (%u)", flag); - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) - zlog (peer->log, LOG_ERR, "LOCAL_PREF attribute must not be flagged as \"partial\" (%u)", flag); + bgp_attr_flags_diagnose (peer, BGP_ATTR_LOCAL_PREF, BGP_ATTR_FLAG_TRANS, flag); return bgp_attr_malformed (peer, BGP_ATTR_LOCAL_PREF, flag, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); @@ -1120,12 +1145,7 @@ bgp_attr_atomic (struct peer *peer, bgp_size_t length, /* Flag checks. */ if ((flag & ~BGP_ATTR_FLAG_EXTLEN) != BGP_ATTR_FLAG_TRANS) { - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) - zlog (peer->log, LOG_ERR, "ATOMIC_AGGREGATE attribute must not be flagged as \"optional\" (%u)", flag); - if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) - zlog (peer->log, LOG_ERR, "ATOMIC_AGGREGATE attribute must be flagged as \"transitive\" (%u)", flag); - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) - zlog (peer->log, LOG_ERR, "ATOMIC_AGGREGATE attribute must not be flagged as \"partial\" (%u)", flag); + bgp_attr_flags_diagnose (peer, BGP_ATTR_ATOMIC_AGGREGATE, BGP_ATTR_FLAG_TRANS, flag); return bgp_attr_malformed (peer, BGP_ATTR_ATOMIC_AGGREGATE, flag, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); @@ -1375,12 +1395,7 @@ bgp_attr_originator_id (struct peer *peer, bgp_size_t length, /* Flag checks. */ if ((flag & ~BGP_ATTR_FLAG_EXTLEN) != BGP_ATTR_FLAG_OPTIONAL) { - if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) - zlog (peer->log, LOG_ERR, "ORIGINATOR_ID attribute must be flagged as \"optional\" (%u)", flag); - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) - zlog (peer->log, LOG_ERR, "ORIGINATOR_ID attribute must not be flagged as \"transitive\" (%u)", flag); - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) - zlog (peer->log, LOG_ERR, "ORIGINATOR_ID attribute must not be flagged as \"partial\" (%u)", flag); + bgp_attr_flags_diagnose (peer, BGP_ATTR_ORIGINATOR_ID, BGP_ATTR_FLAG_OPTIONAL, flag); return bgp_attr_malformed (peer, BGP_ATTR_ORIGINATOR_ID, flag, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); @@ -1414,12 +1429,7 @@ bgp_attr_cluster_list (struct peer *peer, bgp_size_t length, /* Flag checks. */ if ((flag & ~BGP_ATTR_FLAG_EXTLEN) != BGP_ATTR_FLAG_OPTIONAL) { - if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) - zlog (peer->log, LOG_ERR, "CLUSTER_LIST attribute must be flagged as \"optional\" (%u)", flag); - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) - zlog (peer->log, LOG_ERR, "CLUSTER_LIST attribute must not be flagged as \"transitive\" (%u)", flag); - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) - zlog (peer->log, LOG_ERR, "CLUSTER_LIST attribute must not be flagged as \"partial\" (%u)", flag); + bgp_attr_flags_diagnose (peer, BGP_ATTR_CLUSTER_LIST, BGP_ATTR_FLAG_OPTIONAL, flag); return bgp_attr_malformed (peer, BGP_ATTR_CLUSTER_LIST, flag, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); @@ -1463,12 +1473,7 @@ bgp_mp_reach_parse (struct peer *peer, const bgp_size_t length, /* Flag checks. */ if ((flag & ~BGP_ATTR_FLAG_EXTLEN) != BGP_ATTR_FLAG_OPTIONAL) { - if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) - zlog (peer->log, LOG_ERR, "MP_REACH_NLRI attribute must be flagged as \"optional\" (%u)", flag); - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) - zlog (peer->log, LOG_ERR, "MP_REACH_NLRI attribute must not be flagged as \"transitive\" (%u)", flag); - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) - zlog (peer->log, LOG_ERR, "MP_REACH_NLRI attribute must not be flagged as \"partial\" (%u)", flag); + bgp_attr_flags_diagnose (peer, BGP_ATTR_MP_REACH_NLRI, BGP_ATTR_FLAG_OPTIONAL, flag); return bgp_attr_malformed (peer, BGP_ATTR_MP_REACH_NLRI, flag, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); @@ -1606,12 +1611,7 @@ bgp_mp_unreach_parse (struct peer *peer, const bgp_size_t length, /* Flag checks. */ if ((flag & ~BGP_ATTR_FLAG_EXTLEN) != BGP_ATTR_FLAG_OPTIONAL) { - if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) - zlog (peer->log, LOG_ERR, "MP_UNREACH_NLRI attribute must be flagged as \"optional\" (%u)", flag); - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) - zlog (peer->log, LOG_ERR, "MP_UNREACH_NLRI attribute must not be flagged as \"transitive\" (%u)", flag); - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) - zlog (peer->log, LOG_ERR, "MP_UNREACH_NLRI attribute must not be flagged as \"partial\" (%u)", flag); + bgp_attr_flags_diagnose (peer, BGP_ATTR_MP_UNREACH_NLRI, BGP_ATTR_FLAG_OPTIONAL, flag); return bgp_attr_malformed (peer, BGP_ATTR_MP_UNREACH_NLRI, flag, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); -- cgit v1.2.1 From 83a9a2213a73aeb9796b69327c87e89e2d3327ed Mon Sep 17 00:00:00 2001 From: Paul Jakma Date: Sun, 8 Jan 2012 14:15:03 +0000 Subject: bgpd: Improve flag error messages in bgp_attr_aspath * bgpd/bgp_attr.c: (bgp_attr_aspath) error message could be misleading, clearly log what flag was incorrect. (Problem noted in "bgpd: fix error message in bgp_attr_aspath()" in Quagga-RE) --- bgpd/bgp_attr.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index b013c23f..1c13d11c 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -867,7 +867,7 @@ bgp_attr_aspath (struct peer *peer, bgp_size_t length, total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); - /* Flag check. */ + /* Flags check. */ if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) { zlog (peer->log, LOG_ERR, @@ -877,11 +877,20 @@ bgp_attr_aspath (struct peer *peer, bgp_size_t length, startp, total); } - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL) - || ! CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) + if (!CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) + { + zlog (peer->log, LOG_ERR, + "AS_PATH attribute must be flagged as \"transitive\" (%u)", flag); + return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); + } + + if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) { zlog (peer->log, LOG_ERR, - "As-Path attribute flag isn't transitive %d", flag); + "AS_PATH attribute must not be flagged as \"optional\" (%u)", flag); + return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); -- cgit v1.2.1 From f31d6927b2df4340bf9ff1cc9f20d52e168a3766 Mon Sep 17 00:00:00 2001 From: Paul Jakma Date: Sun, 8 Jan 2012 14:17:42 +0000 Subject: bgpd: Fix incorrect attribute type code in call to bgp_attr_malformed --- bgpd/bgp_attr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index 1c13d11c..92c1a09f 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -872,7 +872,7 @@ bgp_attr_aspath (struct peer *peer, bgp_size_t length, { zlog (peer->log, LOG_ERR, "AS_PATH attribute must not be flagged as \"partial\" (%u)", flag); - return bgp_attr_malformed (peer, BGP_ATTR_ORIGIN, flag, + return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); } @@ -989,7 +989,7 @@ bgp_attr_as4_path (struct peer *peer, bgp_size_t length, { zlog (peer->log, LOG_ERR, "As4-Path attribute flag isn't optional/transitive %d", flag); - return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag, + return bgp_attr_malformed (peer, BGP_ATTR_AS4_PATH, flag, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); } -- cgit v1.2.1 From 3ecab4c8549574d09f8d8366098939a8ad3da6c4 Mon Sep 17 00:00:00 2001 From: Paul Jakma Date: Tue, 17 Jan 2012 13:31:33 +0000 Subject: bgpd: consolidate attribute flag checks * bgpd/bgp_attr.c: (attr_flags_values []) array of required flags for attributes, EXTLEN & PARTIAL masked off as "dont care" as appropriate. (bgp_attr_flag_invalid) check if flags may be invalid, according to the above table & RFC rules. (bgp_attr_*) Use bgp_attr_flag_invalid. (bgp_attr_as4_aggregator) ditto, also take startp argument for the NOTIFY data. (bgp_attr_parse) pass startp to bgp_attr_as4_aggregator --- bgpd/bgp_attr.c | 221 ++++++++++++++++++++++++++++++++++---------------------- 1 file changed, 133 insertions(+), 88 deletions(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index 92c1a09f..d8ca831a 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -799,6 +799,92 @@ bgp_attr_flags_diagnose assert (seen); } +/* Required flags for attributes. EXTLEN will be masked off when testing, + * as will PARTIAL for optional+transitive attributes. + */ +const u_int8_t attr_flags_values [] = { + [BGP_ATTR_ORIGIN] = BGP_ATTR_FLAG_TRANS, + [BGP_ATTR_AS_PATH] = BGP_ATTR_FLAG_TRANS, + [BGP_ATTR_NEXT_HOP] = BGP_ATTR_FLAG_TRANS, + [BGP_ATTR_MULTI_EXIT_DISC] = BGP_ATTR_FLAG_OPTIONAL, + [BGP_ATTR_LOCAL_PREF] = BGP_ATTR_FLAG_TRANS, + [BGP_ATTR_ATOMIC_AGGREGATE] = BGP_ATTR_FLAG_TRANS, + [BGP_ATTR_AGGREGATOR] = BGP_ATTR_FLAG_TRANS | BGP_ATTR_FLAG_OPTIONAL, + [BGP_ATTR_COMMUNITIES] = BGP_ATTR_FLAG_TRANS | BGP_ATTR_FLAG_OPTIONAL, + [BGP_ATTR_ORIGINATOR_ID] = BGP_ATTR_FLAG_OPTIONAL, + [BGP_ATTR_CLUSTER_LIST] = BGP_ATTR_FLAG_OPTIONAL, + [BGP_ATTR_MP_REACH_NLRI] = BGP_ATTR_FLAG_OPTIONAL, + [BGP_ATTR_MP_UNREACH_NLRI] = BGP_ATTR_FLAG_OPTIONAL, + [BGP_ATTR_EXT_COMMUNITIES] = BGP_ATTR_FLAG_OPTIONAL | BGP_ATTR_FLAG_TRANS, + [BGP_ATTR_AS4_PATH] = BGP_ATTR_FLAG_OPTIONAL | BGP_ATTR_FLAG_TRANS, + [BGP_ATTR_AS4_AGGREGATOR] = BGP_ATTR_FLAG_OPTIONAL | BGP_ATTR_FLAG_TRANS, +}; +static const size_t attr_flags_values_max = + sizeof (attr_flags_values) / sizeof (attr_flags_values[0]); + +static int +bgp_attr_flag_invalid (struct peer *peer, u_int8_t attr_code, u_int8_t flags) +{ + u_int8_t mask = BGP_ATTR_FLAG_EXTLEN; + + /* there may be attributes we don't know about */ + if (attr_code > attr_flags_values_max) + return 0; + if (attr_flags_values[attr_code] == 0) + return 0; + + /* RFC4271, "For well-known attributes, the Transitive bit MUST be set to + * 1." + */ + if (!CHECK_FLAG (BGP_ATTR_FLAG_OPTIONAL, flags) + && !CHECK_FLAG (BGP_ATTR_FLAG_TRANS, flags)) + { + zlog (peer->log, LOG_ERR, + "%s well-known attributes must have transitive flag set (%x)", + LOOKUP (attr_str, attr_code), flags); + return 1; + } + + /* "For well-known attributes and for optional non-transitive attributes, + * the Partial bit MUST be set to 0." + */ + if (CHECK_FLAG (flags, BGP_ATTR_FLAG_PARTIAL)) + { + if (!CHECK_FLAG (flags, BGP_ATTR_FLAG_OPTIONAL)) + { + zlog (peer->log, LOG_ERR, + "%s well-known attribute " + "must NOT have the partial flag set (%x)", + LOOKUP (attr_str, attr_code), flags); + return 1; + } + if (CHECK_FLAG (flags, BGP_ATTR_FLAG_OPTIONAL) + && !CHECK_FLAG (flags, BGP_ATTR_FLAG_TRANS)) + { + zlog (peer->log, LOG_ERR, + "%s optional + transitive attribute " + "must NOT have the partial flag set (%x)", + LOOKUP (attr_str, attr_code), flags); + return 1; + } + } + + /* Optional transitive attributes may go through speakers that don't + * reocgnise them and set the Partial bit. + */ + if (CHECK_FLAG (flags, BGP_ATTR_FLAG_OPTIONAL) + && CHECK_FLAG (flags, BGP_ATTR_FLAG_TRANS)) + SET_FLAG (mask, BGP_ATTR_FLAG_PARTIAL); + + if ((flags & ~attr_flags_values[attr_code]) + == attr_flags_values[attr_code]) + return 0; + + bgp_attr_flags_diagnose (peer, attr_code, attr_flags_values[attr_code], + flags); + return 1; +} + /* Get origin attribute of the update message. */ static bgp_attr_parse_ret_t bgp_attr_origin (struct peer *peer, bgp_size_t length, @@ -814,11 +900,10 @@ bgp_attr_origin (struct peer *peer, bgp_size_t length, with the Attribute Type Code, then the Error Subcode is set to Attribute Flags Error. The Data field contains the erroneous attribute (type, length and value). */ - if ((flag & ~BGP_ATTR_FLAG_EXTLEN) != BGP_ATTR_FLAG_TRANS) - { - bgp_attr_flags_diagnose (peer, BGP_ATTR_ORIGIN, BGP_ATTR_FLAG_TRANS, flag); - return bgp_attr_malformed (peer, BGP_ATTR_ORIGIN, flag, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); - } + if (bgp_attr_flag_invalid (peer, BGP_ATTR_ORIGIN, flag)) + return bgp_attr_malformed (peer, BGP_ATTR_ORIGIN, flag, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); /* If any recognized attribute has Attribute Length that conflicts with the expected length (based on the attribute type code), then @@ -866,36 +951,11 @@ bgp_attr_aspath (struct peer *peer, bgp_size_t length, bgp_size_t total; total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); - - /* Flags check. */ - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) - { - zlog (peer->log, LOG_ERR, - "AS_PATH attribute must not be flagged as \"partial\" (%u)", flag); - return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - } - - if (!CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) - { - zlog (peer->log, LOG_ERR, - "AS_PATH attribute must be flagged as \"transitive\" (%u)", flag); + + if (bgp_attr_flag_invalid (peer, BGP_ATTR_AS_PATH, flag)) return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); - } - - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) - { - zlog (peer->log, LOG_ERR, - "AS_PATH attribute must not be flagged as \"optional\" (%u)", flag); - - return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - } - /* * peer with AS4 => will get 4Byte ASnums * otherwise, will get 16 Bit @@ -984,16 +1044,11 @@ bgp_attr_as4_path (struct peer *peer, bgp_size_t length, total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); /* Flag check. */ - if (!CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL) - || !CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) - { - zlog (peer->log, LOG_ERR, - "As4-Path attribute flag isn't optional/transitive %d", flag); + if (bgp_attr_flag_invalid (peer, BGP_ATTR_AS4_PATH, flag)) return bgp_attr_malformed (peer, BGP_ATTR_AS4_PATH, flag, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); - } - + *as4_path = aspath_parse (peer->ibuf, length, 1); /* In case of IBGP, length will be zero. */ @@ -1025,11 +1080,10 @@ bgp_attr_nexthop (struct peer *peer, bgp_size_t length, total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); /* Flags check. */ - if ((flag & ~BGP_ATTR_FLAG_EXTLEN) != BGP_ATTR_FLAG_TRANS) - { - bgp_attr_flags_diagnose (peer, BGP_ATTR_NEXT_HOP, BGP_ATTR_FLAG_TRANS, flag); - return bgp_attr_malformed (peer, BGP_ATTR_NEXT_HOP, flag, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); - } + if (bgp_attr_flag_invalid (peer, BGP_ATTR_NEXT_HOP, flag)) + return bgp_attr_malformed (peer, BGP_ATTR_NEXT_HOP, flag, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); /* Check nexthop attribute length. */ if (length != 4) @@ -1075,13 +1129,10 @@ bgp_attr_med (struct peer *peer, bgp_size_t length, total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); /* Flag checks. */ - if ((flag & ~BGP_ATTR_FLAG_EXTLEN) != BGP_ATTR_FLAG_OPTIONAL) - { - bgp_attr_flags_diagnose (peer, BGP_ATTR_MULTI_EXIT_DISC, BGP_ATTR_FLAG_OPTIONAL, flag); + if (bgp_attr_flag_invalid (peer, BGP_ATTR_MULTI_EXIT_DISC, flag)) return bgp_attr_malformed (peer, BGP_ATTR_MULTI_EXIT_DISC, flag, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); - } /* Length check. */ if (length != 4) @@ -1110,13 +1161,11 @@ bgp_attr_local_pref (struct peer *peer, bgp_size_t length, total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); /* Flag checks. */ - if ((flag & ~BGP_ATTR_FLAG_EXTLEN) != BGP_ATTR_FLAG_TRANS) - { - bgp_attr_flags_diagnose (peer, BGP_ATTR_LOCAL_PREF, BGP_ATTR_FLAG_TRANS, flag); + if (bgp_attr_flag_invalid (peer, BGP_ATTR_LOCAL_PREF, flag)) return bgp_attr_malformed (peer, BGP_ATTR_LOCAL_PREF, flag, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); - } + /* Length check. */ if (length != 4) { @@ -1152,14 +1201,11 @@ bgp_attr_atomic (struct peer *peer, bgp_size_t length, total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); /* Flag checks. */ - if ((flag & ~BGP_ATTR_FLAG_EXTLEN) != BGP_ATTR_FLAG_TRANS) - { - bgp_attr_flags_diagnose (peer, BGP_ATTR_ATOMIC_AGGREGATE, BGP_ATTR_FLAG_TRANS, flag); + if (bgp_attr_flag_invalid (peer, BGP_ATTR_ATOMIC_AGGREGATE, flag)) return bgp_attr_malformed (peer, BGP_ATTR_ATOMIC_AGGREGATE, flag, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); - } - + /* Length check. */ if (length != 0) { @@ -1186,22 +1232,11 @@ bgp_attr_aggregator (struct peer *peer, bgp_size_t length, total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); /* Flags check. */ - if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) - { - zlog (peer->log, LOG_ERR, - "AGGREGATOR attribute must be flagged as \"optional\" (%u)", flag); - return bgp_attr_malformed (peer, BGP_ATTR_AGGREGATOR, flag, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - } - if (! CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS)) - { - zlog (peer->log, LOG_ERR, - "AGGREGATOR attribute must be flagged as \"transitive\" (%u)", flag); + if (bgp_attr_flag_invalid (peer, BGP_ATTR_AGGREGATOR, flag)) return bgp_attr_malformed (peer, BGP_ATTR_AGGREGATOR, flag, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); - } + /* peer with AS4 will send 4 Byte AS, peer without will send 2 Byte */ if (CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV)) wantedlen = 8; @@ -1231,8 +1266,11 @@ static bgp_attr_parse_ret_t bgp_attr_as4_aggregator (struct peer *peer, bgp_size_t length, struct attr *attr, u_char flag, as_t *as4_aggregator_as, - struct in_addr *as4_aggregator_addr) + struct in_addr *as4_aggregator_addr, + u_char *startp) { + bgp_size_t total; + if (length != 8) { zlog (peer->log, LOG_ERR, "New Aggregator length is not 8 [%d]", length); @@ -1240,6 +1278,13 @@ bgp_attr_as4_aggregator (struct peer *peer, bgp_size_t length, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, NULL, 0); } + total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); + /* Flags check. */ + if (bgp_attr_flag_invalid (peer, BGP_ATTR_AS4_AGGREGATOR, flag)) + return bgp_attr_malformed (peer, BGP_ATTR_AS4_AGGREGATOR, flag, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); + *as4_aggregator_as = stream_getl (peer->ibuf); as4_aggregator_addr->s_addr = stream_get_ipv4 (peer->ibuf); @@ -1377,6 +1422,12 @@ bgp_attr_community (struct peer *peer, bgp_size_t length, return BGP_ATTR_PARSE_PROCEED; } + /* Flags check. */ + if (bgp_attr_flag_invalid (peer, BGP_ATTR_COMMUNITIES, flag)) + return bgp_attr_malformed (peer, BGP_ATTR_COMMUNITIES, flag, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); + attr->community = community_parse ((u_int32_t *)stream_pnt (peer->ibuf), length); @@ -1402,13 +1453,10 @@ bgp_attr_originator_id (struct peer *peer, bgp_size_t length, total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); /* Flag checks. */ - if ((flag & ~BGP_ATTR_FLAG_EXTLEN) != BGP_ATTR_FLAG_OPTIONAL) - { - bgp_attr_flags_diagnose (peer, BGP_ATTR_ORIGINATOR_ID, BGP_ATTR_FLAG_OPTIONAL, flag); + if (bgp_attr_flag_invalid (peer, BGP_ATTR_ORIGINATOR_ID, flag)) return bgp_attr_malformed (peer, BGP_ATTR_ORIGINATOR_ID, flag, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); - } /* Length check. */ if (length != 4) { @@ -1436,13 +1484,10 @@ bgp_attr_cluster_list (struct peer *peer, bgp_size_t length, total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); /* Flag checks. */ - if ((flag & ~BGP_ATTR_FLAG_EXTLEN) != BGP_ATTR_FLAG_OPTIONAL) - { - bgp_attr_flags_diagnose (peer, BGP_ATTR_CLUSTER_LIST, BGP_ATTR_FLAG_OPTIONAL, flag); + if (bgp_attr_flag_invalid (peer, BGP_ATTR_CLUSTER_LIST, flag)) return bgp_attr_malformed (peer, BGP_ATTR_CLUSTER_LIST, flag, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); - } /* Check length. */ if (length % 4) { @@ -1480,13 +1525,10 @@ bgp_mp_reach_parse (struct peer *peer, const bgp_size_t length, total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); /* Flag checks. */ - if ((flag & ~BGP_ATTR_FLAG_EXTLEN) != BGP_ATTR_FLAG_OPTIONAL) - { - bgp_attr_flags_diagnose (peer, BGP_ATTR_MP_REACH_NLRI, BGP_ATTR_FLAG_OPTIONAL, flag); + if (bgp_attr_flag_invalid (peer, BGP_ATTR_MP_REACH_NLRI, flag)) return bgp_attr_malformed (peer, BGP_ATTR_MP_REACH_NLRI, flag, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); - } /* Set end of packet. */ s = BGP_INPUT(peer); start = stream_get_getp(s); @@ -1618,13 +1660,10 @@ bgp_mp_unreach_parse (struct peer *peer, const bgp_size_t length, total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); /* Flag checks. */ - if ((flag & ~BGP_ATTR_FLAG_EXTLEN) != BGP_ATTR_FLAG_OPTIONAL) - { - bgp_attr_flags_diagnose (peer, BGP_ATTR_MP_UNREACH_NLRI, BGP_ATTR_FLAG_OPTIONAL, flag); + if (bgp_attr_flag_invalid (peer, BGP_ATTR_MP_UNREACH_NLRI, flag)) return bgp_attr_malformed (peer, BGP_ATTR_MP_UNREACH_NLRI, flag, BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, startp, total); - } s = peer->ibuf; @@ -1670,6 +1709,12 @@ bgp_attr_ext_communities (struct peer *peer, bgp_size_t length, return BGP_ATTR_PARSE_PROCEED; } + /* Flags check. */ + if (bgp_attr_flag_invalid (peer, BGP_ATTR_EXT_COMMUNITIES, flag)) + return bgp_attr_malformed (peer, BGP_ATTR_EXT_COMMUNITIES, flag, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + startp, total); + (bgp_attr_extra_get (attr))->ecommunity = ecommunity_parse ((u_int8_t *)stream_pnt (peer->ibuf), length); /* XXX: fix ecommunity_parse to use stream API */ @@ -1884,7 +1929,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, case BGP_ATTR_AS4_AGGREGATOR: ret = bgp_attr_as4_aggregator (peer, length, attr, flag, &as4_aggregator, - &as4_aggregator_addr); + &as4_aggregator_addr, startp); break; case BGP_ATTR_COMMUNITIES: ret = bgp_attr_community (peer, length, attr, flag, startp); -- cgit v1.2.1 From 835315bfb49bff2b2fb354f2075c6d6693c2a151 Mon Sep 17 00:00:00 2001 From: Paul Jakma Date: Wed, 18 Jan 2012 12:28:30 +0000 Subject: bgpd: Move up flag-check calls, parcel up attr-parser args, and other cleanups * bgp_attr.h: (struct bgp_attr_parser_args) Attribute parsing context, containing common arguments. * bgp_attr.c: (general) Move the bgp_attr_flag_invalid flag-check calls up, out of each individual attr parser function, to be done once in attr_parse. Similarly move the calculation of the 'total' attribute length field up to attr_parse. Bundle together common arguments to attr-parsing functions and helpers into (struct bgp_attr_parser_args), so it can be passed by reference down the stack & also de-clutter the argument lists & make it easier to add/modify the context for attr-parsing - add local const aliases to avoid modifying body of code too much. This also should help avoid cut & paste errors, where calls to helpers with hard-coded attribute types are pasted to other functions but the code isn't changed. (bgp_attr_flags_diagnose) as above. (bgp_attr_flag_invalid) as above. (bgp_attr_{origin,aspath,as4_path,nexthop,med,local_pref,atomic}) as above. (bgp_attr_{aggregator,as4_aggregator,community,originator_id}) as above (bgp_attr_{cluster_list,ext_communities},bgp_mp_{un,}reach_parse) as above (bgp_attr_unknown) as above. (bgp_attr_malformed) as above. Also, startp and length have to be special-cased, because whether or not to send attribute data depends on the particular error - a separate length argument, distinct from args->length, indicates whether or not the attribute data should be sent in the NOTIFY. (bgp_attr_aspath_check) Call to bgp_attr_malformed is wrong here, there is no attribute parsing context - e.g. the 'flag' argument is unlikely to be right, remove it. Explicitly handle the error instead. (bgp_attr_munge_as4_attrs) Flag argument is pointless. As the comment notes, the check here is pointless as AS_PATH presence already checked elsewhere. (bgp_attr_parse) Do bgp_attr_flag_invalid call here. Use (struct bgp_attr_parser_args) for args to attr parser functions. Remove out-of-context 'flag' argument to as4 checking functions. --- bgpd/bgp_attr.c | 467 +++++++++++++++++++++++--------------------------------- 1 file changed, 194 insertions(+), 273 deletions(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index d8ca831a..b02cfee3 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -710,9 +710,17 @@ bgp_attr_flush (struct attr *attr) * introduced by the sending neighbour. */ static bgp_attr_parse_ret_t -bgp_attr_malformed (struct peer *peer, u_char type, u_char flag, - u_char subcode, u_char *startp, bgp_size_t length) -{ +bgp_attr_malformed (struct bgp_attr_parser_args *args, u_char subcode, + bgp_size_t length) +{ + struct peer *const peer = args->peer; + const u_int8_t flags = args->flags; + /* startp and length must be special-cased, as whether or not to + * send the attribute data with the NOTIFY depends on the error, + * the caller therefore signals this with the seperate length argument + */ + u_char *startp = (length > 0 ? args->startp : NULL); + /* Only relax error handling for eBGP peers */ if (peer_sort (peer) != BGP_PEER_EBGP) { @@ -722,7 +730,7 @@ bgp_attr_malformed (struct peer *peer, u_char type, u_char flag, } - switch (type) { + switch (args->type) { /* where an optional attribute is inconsequential, e.g. it does not affect * route selection, and can be safely ignored then any such attributes * which are malformed should just be ignored and the route processed as @@ -756,9 +764,9 @@ bgp_attr_malformed (struct peer *peer, u_char type, u_char flag, * the whole session to be reset. Instead treat it as a withdrawal * of the routes, if possible. */ - if (CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS) - && CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL) - && CHECK_FLAG (flag, BGP_ATTR_FLAG_PARTIAL)) + if (CHECK_FLAG (flags, BGP_ATTR_FLAG_TRANS) + && CHECK_FLAG (flags, BGP_ATTR_FLAG_OPTIONAL) + && CHECK_FLAG (flags, BGP_ATTR_FLAG_PARTIAL)) return BGP_ATTR_PARSE_WITHDRAW; /* default to reset */ @@ -771,16 +779,14 @@ bgp_attr_malformed (struct peer *peer, u_char type, u_char flag, being diagnosed is defined by RFC as either a "well-known" or an "optional, non-transitive" attribute. */ static void -bgp_attr_flags_diagnose -( - struct peer * peer, - const u_int8_t attr_code, - u_int8_t desired_flags, /* how RFC says it must be */ - u_int8_t real_flags /* on wire */ +bgp_attr_flags_diagnose (struct bgp_attr_parser_args *args, + u_int8_t desired_flags /* how RFC says it must be */ ) { u_char seen = 0, i; - + u_char real_flags = args->flags; + const u_int8_t attr_code = args->type; + desired_flags &= ~BGP_ATTR_FLAG_EXTLEN; real_flags &= ~BGP_ATTR_FLAG_EXTLEN; for (i = 0; i <= 2; i++) /* O,T,P, but not E */ @@ -790,7 +796,7 @@ bgp_attr_flags_diagnose CHECK_FLAG (real_flags, attr_flag_str[i].key) ) { - zlog (peer->log, LOG_ERR, "%s attribute must%s be flagged as \"%s\"", + zlog (args->peer->log, LOG_ERR, "%s attribute must%s be flagged as \"%s\"", LOOKUP (attr_str, attr_code), CHECK_FLAG (desired_flags, attr_flag_str[i].key) ? "" : " not", attr_flag_str[i].str); @@ -823,9 +829,12 @@ static const size_t attr_flags_values_max = sizeof (attr_flags_values) / sizeof (attr_flags_values[0]); static int -bgp_attr_flag_invalid (struct peer *peer, u_int8_t attr_code, u_int8_t flags) +bgp_attr_flag_invalid (struct bgp_attr_parser_args *args) { u_int8_t mask = BGP_ATTR_FLAG_EXTLEN; + const u_int8_t flags = args->flags; + const u_int8_t attr_code = args->type; + struct peer *const peer = args->peer; /* there may be attributes we don't know about */ if (attr_code > attr_flags_values_max) @@ -880,31 +889,18 @@ bgp_attr_flag_invalid (struct peer *peer, u_int8_t attr_code, u_int8_t flags) == attr_flags_values[attr_code]) return 0; - bgp_attr_flags_diagnose (peer, attr_code, attr_flags_values[attr_code], - flags); + bgp_attr_flags_diagnose (args, attr_flags_values[attr_code]); return 1; } /* Get origin attribute of the update message. */ static bgp_attr_parse_ret_t -bgp_attr_origin (struct peer *peer, bgp_size_t length, - struct attr *attr, u_char flag, u_char *startp) +bgp_attr_origin (struct bgp_attr_parser_args *args) { - bgp_size_t total; - - /* total is entire attribute length include Attribute Flags (1), - Attribute Type code (1) and Attribute length (1 or 2). */ - total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); - - /* If any recognized attribute has Attribute Flags that conflict - with the Attribute Type Code, then the Error Subcode is set to - Attribute Flags Error. The Data field contains the erroneous - attribute (type, length and value). */ - if (bgp_attr_flag_invalid (peer, BGP_ATTR_ORIGIN, flag)) - return bgp_attr_malformed (peer, BGP_ATTR_ORIGIN, flag, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - + struct peer *const peer = args->peer; + struct attr *const attr = args->attr; + const bgp_size_t length = args->length; + /* If any recognized attribute has Attribute Length that conflicts with the expected length (based on the attribute type code), then the Error Subcode is set to Attribute Length Error. The Data @@ -914,9 +910,9 @@ bgp_attr_origin (struct peer *peer, bgp_size_t length, { zlog (peer->log, LOG_ERR, "Origin attribute length is not one %d", length); - return bgp_attr_malformed (peer, BGP_ATTR_ORIGIN, flag, + return bgp_attr_malformed (args, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, - startp, total); + args->total); } /* Fetch origin attribute. */ @@ -931,9 +927,9 @@ bgp_attr_origin (struct peer *peer, bgp_size_t length, { zlog (peer->log, LOG_ERR, "Origin attribute value is invalid %d", attr->origin); - return bgp_attr_malformed (peer, BGP_ATTR_ORIGIN, flag, + return bgp_attr_malformed (args, BGP_NOTIFY_UPDATE_INVAL_ORIGIN, - startp, total); + args->total); } /* Set oring attribute flag. */ @@ -945,17 +941,12 @@ bgp_attr_origin (struct peer *peer, bgp_size_t length, /* Parse AS path information. This function is wrapper of aspath_parse. */ static int -bgp_attr_aspath (struct peer *peer, bgp_size_t length, - struct attr *attr, u_char flag, u_char *startp) +bgp_attr_aspath (struct bgp_attr_parser_args *args) { - bgp_size_t total; - - total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); + struct attr *const attr = args->attr; + struct peer *const peer = args->peer; + const bgp_size_t length = args->length; - if (bgp_attr_flag_invalid (peer, BGP_ATTR_AS_PATH, flag)) - return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); /* * peer with AS4 => will get 4Byte ASnums * otherwise, will get 16 Bit @@ -969,9 +960,7 @@ bgp_attr_aspath (struct peer *peer, bgp_size_t length, zlog (peer->log, LOG_ERR, "Malformed AS path from %s, length is %d", peer->host, length); - return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag, - BGP_NOTIFY_UPDATE_MAL_AS_PATH, - NULL, 0); + return bgp_attr_malformed (args, BGP_NOTIFY_UPDATE_MAL_AS_PATH, 0); } /* Set aspath attribute flag. */ @@ -981,7 +970,7 @@ bgp_attr_aspath (struct peer *peer, bgp_size_t length, } static bgp_attr_parse_ret_t -bgp_attr_aspath_check (struct peer *peer, struct attr *attr, u_char flag) +bgp_attr_aspath_check (struct peer *const peer, struct attr *const attr) { /* These checks were part of bgp_attr_aspath, but with * as4 we should to check aspath things when @@ -1000,9 +989,9 @@ bgp_attr_aspath_check (struct peer *peer, struct attr *attr, u_char flag) (peer_sort (peer) == BGP_PEER_EBGP && aspath_confed_check (attr->aspath))) { zlog (peer->log, LOG_ERR, "Malformed AS path from %s", peer->host); - return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag, - BGP_NOTIFY_UPDATE_MAL_AS_PATH, - NULL, 0); + bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, + BGP_NOTIFY_UPDATE_MAL_AS_PATH); + return BGP_ATTR_PARSE_ERROR; } /* First AS check for EBGP. */ @@ -1013,9 +1002,9 @@ bgp_attr_aspath_check (struct peer *peer, struct attr *attr, u_char flag) { zlog (peer->log, LOG_ERR, "%s incorrect first AS (must be %u)", peer->host, peer->as); - return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag, - BGP_NOTIFY_UPDATE_MAL_AS_PATH, - NULL, 0); + bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, + BGP_NOTIFY_UPDATE_MAL_AS_PATH); + return BGP_ATTR_PARSE_ERROR; } } @@ -1035,19 +1024,11 @@ bgp_attr_aspath_check (struct peer *peer, struct attr *attr, u_char flag) /* Parse AS4 path information. This function is another wrapper of aspath_parse. */ static int -bgp_attr_as4_path (struct peer *peer, bgp_size_t length, - struct attr *attr, u_char flag, u_char *startp, - struct aspath **as4_path) +bgp_attr_as4_path (struct bgp_attr_parser_args *args, struct aspath **as4_path) { - bgp_size_t total; - - total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); - - /* Flag check. */ - if (bgp_attr_flag_invalid (peer, BGP_ATTR_AS4_PATH, flag)) - return bgp_attr_malformed (peer, BGP_ATTR_AS4_PATH, flag, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); + struct peer *const peer = args->peer; + struct attr *const attr = args->attr; + const bgp_size_t length = args->length; *as4_path = aspath_parse (peer->ibuf, length, 1); @@ -1057,9 +1038,9 @@ bgp_attr_as4_path (struct peer *peer, bgp_size_t length, zlog (peer->log, LOG_ERR, "Malformed AS4 path from %s, length is %d", peer->host, length); - return bgp_attr_malformed (peer, BGP_ATTR_AS4_PATH, flag, + return bgp_attr_malformed (args, BGP_NOTIFY_UPDATE_MAL_AS_PATH, - NULL, 0); + 0); } /* Set aspath attribute flag. */ @@ -1071,29 +1052,23 @@ bgp_attr_as4_path (struct peer *peer, bgp_size_t length, /* Nexthop attribute. */ static bgp_attr_parse_ret_t -bgp_attr_nexthop (struct peer *peer, bgp_size_t length, - struct attr *attr, u_char flag, u_char *startp) +bgp_attr_nexthop (struct bgp_attr_parser_args *args) { - bgp_size_t total; + struct peer *const peer = args->peer; + struct attr *const attr = args->attr; + const bgp_size_t length = args->length; + in_addr_t nexthop_h, nexthop_n; - total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); - - /* Flags check. */ - if (bgp_attr_flag_invalid (peer, BGP_ATTR_NEXT_HOP, flag)) - return bgp_attr_malformed (peer, BGP_ATTR_NEXT_HOP, flag, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - /* Check nexthop attribute length. */ if (length != 4) { zlog (peer->log, LOG_ERR, "Nexthop attribute length isn't four [%d]", length); - return bgp_attr_malformed (peer, BGP_ATTR_NEXT_HOP, flag, + return bgp_attr_malformed (args, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, - startp, total); + args->total); } /* According to section 6.3 of RFC4271, syntactically incorrect NEXT_HOP @@ -1108,9 +1083,9 @@ bgp_attr_nexthop (struct peer *peer, bgp_size_t length, char buf[INET_ADDRSTRLEN]; inet_ntop (AF_INET, &nexthop_h, buf, INET_ADDRSTRLEN); zlog (peer->log, LOG_ERR, "Martian nexthop %s", buf); - return bgp_attr_malformed (peer, BGP_ATTR_NEXT_HOP, flag, + return bgp_attr_malformed (args, BGP_NOTIFY_UPDATE_INVAL_NEXT_HOP, - startp, total); + args->total); } attr->nexthop.s_addr = nexthop_n; @@ -1121,28 +1096,21 @@ bgp_attr_nexthop (struct peer *peer, bgp_size_t length, /* MED atrribute. */ static bgp_attr_parse_ret_t -bgp_attr_med (struct peer *peer, bgp_size_t length, - struct attr *attr, u_char flag, u_char *startp) +bgp_attr_med (struct bgp_attr_parser_args *args) { - bgp_size_t total; - - total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); - - /* Flag checks. */ - if (bgp_attr_flag_invalid (peer, BGP_ATTR_MULTI_EXIT_DISC, flag)) - return bgp_attr_malformed (peer, BGP_ATTR_MULTI_EXIT_DISC, flag, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - + struct peer *const peer = args->peer; + struct attr *const attr = args->attr; + const bgp_size_t length = args->length; + /* Length check. */ if (length != 4) { zlog (peer->log, LOG_ERR, "MED attribute length isn't four [%d]", length); - return bgp_attr_malformed (peer, BGP_ATTR_MULTI_EXIT_DISC, flag, + return bgp_attr_malformed (args, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, - startp, total); + args->total); } attr->med = stream_getl (peer->ibuf); @@ -1154,25 +1122,20 @@ bgp_attr_med (struct peer *peer, bgp_size_t length, /* Local preference attribute. */ static bgp_attr_parse_ret_t -bgp_attr_local_pref (struct peer *peer, bgp_size_t length, - struct attr *attr, u_char flag, u_char *startp) +bgp_attr_local_pref (struct bgp_attr_parser_args *args) { - bgp_size_t total; - - total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); - /* Flag checks. */ - if (bgp_attr_flag_invalid (peer, BGP_ATTR_LOCAL_PREF, flag)) - return bgp_attr_malformed (peer, BGP_ATTR_LOCAL_PREF, flag, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); + struct peer *const peer = args->peer; + struct attr *const attr = args->attr; + const bgp_size_t length = args->length; /* Length check. */ if (length != 4) { - zlog (peer->log, LOG_ERR, "LOCAL_PREF attribute length isn't 4 [%u]", length); - return bgp_attr_malformed (peer, BGP_ATTR_LOCAL_PREF, flag, + zlog (peer->log, LOG_ERR, "LOCAL_PREF attribute length isn't 4 [%u]", + length); + return bgp_attr_malformed (args, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, - startp, total); + args->total); } /* If it is contained in an UPDATE message that is received from an @@ -1194,25 +1157,20 @@ bgp_attr_local_pref (struct peer *peer, bgp_size_t length, /* Atomic aggregate. */ static int -bgp_attr_atomic (struct peer *peer, bgp_size_t length, - struct attr *attr, u_char flag, u_char *startp) +bgp_attr_atomic (struct bgp_attr_parser_args *args) { - bgp_size_t total; - - total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); - /* Flag checks. */ - if (bgp_attr_flag_invalid (peer, BGP_ATTR_ATOMIC_AGGREGATE, flag)) - return bgp_attr_malformed (peer, BGP_ATTR_ATOMIC_AGGREGATE, flag, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); + struct peer *const peer = args->peer; + struct attr *const attr = args->attr; + const bgp_size_t length = args->length; /* Length check. */ if (length != 0) { - zlog (peer->log, LOG_ERR, "ATOMIC_AGGREGATE attribute length isn't 0 [%u]", length); - return bgp_attr_malformed (peer, BGP_ATTR_ATOMIC_AGGREGATE, flag, + zlog (peer->log, LOG_ERR, "ATOMIC_AGGREGATE attribute length isn't 0 [%u]", + length); + return bgp_attr_malformed (args, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, - startp, total); + args->total); } /* Set atomic aggregate flag. */ @@ -1223,19 +1181,14 @@ bgp_attr_atomic (struct peer *peer, bgp_size_t length, /* Aggregator attribute */ static int -bgp_attr_aggregator (struct peer *peer, bgp_size_t length, - struct attr *attr, u_char flag, u_char *startp) +bgp_attr_aggregator (struct bgp_attr_parser_args *args) { + struct peer *const peer = args->peer; + struct attr *const attr = args->attr; + const bgp_size_t length = args->length; + int wantedlen = 6; struct attr_extra *attre = bgp_attr_extra_get (attr); - bgp_size_t total; - - total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); - /* Flags check. */ - if (bgp_attr_flag_invalid (peer, BGP_ATTR_AGGREGATOR, flag)) - return bgp_attr_malformed (peer, BGP_ATTR_AGGREGATOR, flag, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); /* peer with AS4 will send 4 Byte AS, peer without will send 2 Byte */ if (CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV)) @@ -1243,10 +1196,11 @@ bgp_attr_aggregator (struct peer *peer, bgp_size_t length, if (length != wantedlen) { - zlog (peer->log, LOG_ERR, "AGGREGATOR attribute length isn't %u [%u]", wantedlen, length); - return bgp_attr_malformed (peer, BGP_ATTR_AGGREGATOR, flag, + zlog (peer->log, LOG_ERR, "AGGREGATOR attribute length isn't %u [%u]", + wantedlen, length); + return bgp_attr_malformed (args, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, - startp, total); + args->total); } if ( CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV ) ) @@ -1263,27 +1217,22 @@ bgp_attr_aggregator (struct peer *peer, bgp_size_t length, /* New Aggregator attribute */ static bgp_attr_parse_ret_t -bgp_attr_as4_aggregator (struct peer *peer, bgp_size_t length, - struct attr *attr, u_char flag, - as_t *as4_aggregator_as, - struct in_addr *as4_aggregator_addr, - u_char *startp) +bgp_attr_as4_aggregator (struct bgp_attr_parser_args *args, + as_t *as4_aggregator_as, + struct in_addr *as4_aggregator_addr) { - bgp_size_t total; - + struct peer *const peer = args->peer; + struct attr *const attr = args->attr; + const bgp_size_t length = args->length; + if (length != 8) { - zlog (peer->log, LOG_ERR, "New Aggregator length is not 8 [%d]", length); - return bgp_attr_malformed (peer, BGP_ATTR_AS4_AGGREGATOR, flag, + zlog (peer->log, LOG_ERR, "New Aggregator length is not 8 [%d]", + length); + return bgp_attr_malformed (args, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, - NULL, 0); + 0); } - total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); - /* Flags check. */ - if (bgp_attr_flag_invalid (peer, BGP_ATTR_AS4_AGGREGATOR, flag)) - return bgp_attr_malformed (peer, BGP_ATTR_AS4_AGGREGATOR, flag, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); *as4_aggregator_as = stream_getl (peer->ibuf); as4_aggregator_addr->s_addr = stream_get_ipv4 (peer->ibuf); @@ -1296,7 +1245,8 @@ bgp_attr_as4_aggregator (struct peer *peer, bgp_size_t length, /* Munge Aggregator and New-Aggregator, AS_PATH and NEW_AS_PATH. */ static bgp_attr_parse_ret_t -bgp_attr_munge_as4_attrs (struct peer *peer, struct attr *attr, u_char flag, +bgp_attr_munge_as4_attrs (struct peer *const peer, + struct attr *const attr, struct aspath *as4_path, as_t as4_aggregator, struct in_addr *as4_aggregator_addr) { @@ -1325,24 +1275,6 @@ bgp_attr_munge_as4_attrs (struct peer *peer, struct attr *attr, u_char flag, return BGP_ATTR_PARSE_PROCEED; } - if (attr->flag & (ATTR_FLAG_BIT (BGP_ATTR_AS4_PATH)) - && !(attr->flag & (ATTR_FLAG_BIT (BGP_ATTR_AS_PATH)))) - { - /* Hu? This is not supposed to happen at all! - * got as4_path and no aspath, - * This should already - * have been handled by 'well known attributes missing' - * But... yeah, paranoia - * Take this as a "malformed attribute" - */ - zlog (peer->log, LOG_ERR, - "%s BGP not AS4 capable peer sent AS4_PATH but" - " no AS_PATH, cant do anything here", peer->host); - return bgp_attr_malformed (peer, BGP_ATTR_AS_PATH, flag, - BGP_NOTIFY_UPDATE_MAL_ATTR, - NULL, 0); - } - /* We have a asn16 peer. First, look for AS4_AGGREGATOR * because that may override AS4_PATH */ @@ -1410,11 +1342,11 @@ bgp_attr_munge_as4_attrs (struct peer *peer, struct attr *attr, u_char flag, /* Community attribute. */ static bgp_attr_parse_ret_t -bgp_attr_community (struct peer *peer, bgp_size_t length, - struct attr *attr, u_char flag, u_char *startp) +bgp_attr_community (struct bgp_attr_parser_args *args) { - bgp_size_t total - = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); + struct peer *const peer = args->peer; + struct attr *const attr = args->attr; + const bgp_size_t length = args->length; if (length == 0) { @@ -1422,12 +1354,6 @@ bgp_attr_community (struct peer *peer, bgp_size_t length, return BGP_ATTR_PARSE_PROCEED; } - /* Flags check. */ - if (bgp_attr_flag_invalid (peer, BGP_ATTR_COMMUNITIES, flag)) - return bgp_attr_malformed (peer, BGP_ATTR_COMMUNITIES, flag, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - attr->community = community_parse ((u_int32_t *)stream_pnt (peer->ibuf), length); @@ -1435,9 +1361,9 @@ bgp_attr_community (struct peer *peer, bgp_size_t length, stream_forward_getp (peer->ibuf, length); if (!attr->community) - return bgp_attr_malformed (peer, BGP_ATTR_COMMUNITIES, flag, + return bgp_attr_malformed (args, BGP_NOTIFY_UPDATE_OPT_ATTR_ERR, - startp, total); + args->total); attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_COMMUNITIES); @@ -1446,25 +1372,20 @@ bgp_attr_community (struct peer *peer, bgp_size_t length, /* Originator ID attribute. */ static bgp_attr_parse_ret_t -bgp_attr_originator_id (struct peer *peer, bgp_size_t length, - struct attr *attr, u_char flag, u_char *startp) +bgp_attr_originator_id (struct bgp_attr_parser_args *args) { - bgp_size_t total; - - total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); - /* Flag checks. */ - if (bgp_attr_flag_invalid (peer, BGP_ATTR_ORIGINATOR_ID, flag)) - return bgp_attr_malformed (peer, BGP_ATTR_ORIGINATOR_ID, flag, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); + struct peer *const peer = args->peer; + struct attr *const attr = args->attr; + const bgp_size_t length = args->length; + /* Length check. */ if (length != 4) { zlog (peer->log, LOG_ERR, "Bad originator ID length %d", length); - return bgp_attr_malformed (peer, BGP_ATTR_ORIGINATOR_ID, flag, + return bgp_attr_malformed (args, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, - startp, total); + args->total); } (bgp_attr_extra_get (attr))->originator_id.s_addr @@ -1477,25 +1398,19 @@ bgp_attr_originator_id (struct peer *peer, bgp_size_t length, /* Cluster list attribute. */ static bgp_attr_parse_ret_t -bgp_attr_cluster_list (struct peer *peer, bgp_size_t length, - struct attr *attr, u_char flag, u_char *startp) +bgp_attr_cluster_list (struct bgp_attr_parser_args *args) { - bgp_size_t total; - - total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); - /* Flag checks. */ - if (bgp_attr_flag_invalid (peer, BGP_ATTR_CLUSTER_LIST, flag)) - return bgp_attr_malformed (peer, BGP_ATTR_CLUSTER_LIST, flag, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); + struct peer *const peer = args->peer; + struct attr *const attr = args->attr; + const bgp_size_t length = args->length; + /* Check length. */ if (length % 4) { zlog (peer->log, LOG_ERR, "Bad cluster list length %d", length); - return bgp_attr_malformed (peer, BGP_ATTR_CLUSTER_LIST, flag, - BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, - startp, total); + return bgp_attr_malformed (args, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, + args->total); } (bgp_attr_extra_get (attr))->cluster @@ -1511,8 +1426,8 @@ bgp_attr_cluster_list (struct peer *peer, bgp_size_t length, /* Multiprotocol reachability information parse. */ int -bgp_mp_reach_parse (struct peer *peer, const bgp_size_t length, - struct attr *attr, const u_char flag, u_char *startp, struct bgp_nlri *mp_update) +bgp_mp_reach_parse (struct bgp_attr_parser_args *args, + struct bgp_nlri *mp_update) { afi_t afi; safi_t safi; @@ -1520,15 +1435,11 @@ bgp_mp_reach_parse (struct peer *peer, const bgp_size_t length, size_t start; int ret; struct stream *s; + struct peer *const peer = args->peer; + struct attr *const attr = args->attr; + const bgp_size_t length = args->length; struct attr_extra *attre = bgp_attr_extra_get(attr); - bgp_size_t total; - - total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); - /* Flag checks. */ - if (bgp_attr_flag_invalid (peer, BGP_ATTR_MP_REACH_NLRI, flag)) - return bgp_attr_malformed (peer, BGP_ATTR_MP_REACH_NLRI, flag, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); + /* Set end of packet. */ s = BGP_INPUT(peer); start = stream_get_getp(s); @@ -1647,8 +1558,7 @@ bgp_mp_reach_parse (struct peer *peer, const bgp_size_t length, /* Multiprotocol unreachable parse */ int -bgp_mp_unreach_parse (struct peer *peer, const bgp_size_t length, - const u_char flag, u_char *startp, +bgp_mp_unreach_parse (struct bgp_attr_parser_args *args, struct bgp_nlri *mp_withdraw) { struct stream *s; @@ -1656,14 +1566,8 @@ bgp_mp_unreach_parse (struct peer *peer, const bgp_size_t length, safi_t safi; u_int16_t withdraw_len; int ret; - bgp_size_t total; - - total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); - /* Flag checks. */ - if (bgp_attr_flag_invalid (peer, BGP_ATTR_MP_UNREACH_NLRI, flag)) - return bgp_attr_malformed (peer, BGP_ATTR_MP_UNREACH_NLRI, flag, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); + struct peer *const peer = args->peer; + const bgp_size_t length = args->length; s = peer->ibuf; @@ -1695,11 +1599,11 @@ bgp_mp_unreach_parse (struct peer *peer, const bgp_size_t length, /* Extended Community attribute. */ static bgp_attr_parse_ret_t -bgp_attr_ext_communities (struct peer *peer, bgp_size_t length, - struct attr *attr, u_char flag, u_char *startp) +bgp_attr_ext_communities (struct bgp_attr_parser_args *args) { - bgp_size_t total - = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); + struct peer *const peer = args->peer; + struct attr *const attr = args->attr; + const bgp_size_t length = args->length; if (length == 0) { @@ -1709,21 +1613,15 @@ bgp_attr_ext_communities (struct peer *peer, bgp_size_t length, return BGP_ATTR_PARSE_PROCEED; } - /* Flags check. */ - if (bgp_attr_flag_invalid (peer, BGP_ATTR_EXT_COMMUNITIES, flag)) - return bgp_attr_malformed (peer, BGP_ATTR_EXT_COMMUNITIES, flag, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - startp, total); - (bgp_attr_extra_get (attr))->ecommunity = ecommunity_parse ((u_int8_t *)stream_pnt (peer->ibuf), length); /* XXX: fix ecommunity_parse to use stream API */ stream_forward_getp (peer->ibuf, length); if (!attr->extra->ecommunity) - return bgp_attr_malformed (peer, BGP_ATTR_EXT_COMMUNITIES, - flag, BGP_NOTIFY_UPDATE_OPT_ATTR_ERR, - startp, total); + return bgp_attr_malformed (args, + BGP_NOTIFY_UPDATE_OPT_ATTR_ERR, + args->total); attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_EXT_COMMUNITIES); @@ -1732,12 +1630,18 @@ bgp_attr_ext_communities (struct peer *peer, bgp_size_t length, /* BGP unknown attribute treatment. */ static bgp_attr_parse_ret_t -bgp_attr_unknown (struct peer *peer, struct attr *attr, u_char flag, - u_char type, bgp_size_t length, u_char *startp) +bgp_attr_unknown (struct bgp_attr_parser_args *args) { bgp_size_t total; struct transit *transit; struct attr_extra *attre; + struct peer *const peer = args->peer; + struct attr *const attr = args->attr; + u_char *const startp = args->startp; + const u_char type = args->type; + const u_char flag = args->flags; + const bgp_size_t length = args->length; + if (BGP_DEBUG (normal, NORMAL)) zlog_debug ("%s Unknown attribute is received (type %d, length %d)", @@ -1750,18 +1654,15 @@ bgp_attr_unknown (struct peer *peer, struct attr *attr, u_char flag, /* Forward read pointer of input stream. */ stream_forward_getp (peer->ibuf, length); - /* Adjest total length to include type and length. */ - total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3); - /* If any of the mandatory well-known attributes are not recognized, then the Error Subcode is set to Unrecognized Well-known Attribute. The Data field contains the unrecognized attribute (type, length and value). */ if (!CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)) { - return bgp_attr_malformed (peer, type, flag, + return bgp_attr_malformed (args, BGP_NOTIFY_UPDATE_UNREC_ATTR, - startp, total); + args->total); } /* Unrecognized non-transitive optional attributes must be quietly @@ -1858,7 +1759,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR); return BGP_ATTR_PARSE_ERROR; } - + /* Check extended attribue length bit. */ if (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN)) length = stream_getw (BGP_INPUT (peer)); @@ -1898,59 +1799,79 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR); return BGP_ATTR_PARSE_ERROR; } + + struct bgp_attr_parser_args attr_args = { + .peer = peer, + .length = length, + .attr = attr, + .type = type, + .flags = flag, + .startp = startp, + .total = attr_endp - startp, + }; + + + /* If any recognized attribute has Attribute Flags that conflict + with the Attribute Type Code, then the Error Subcode is set to + Attribute Flags Error. The Data field contains the erroneous + attribute (type, length and value). */ + if (bgp_attr_flag_invalid (&attr_args)) + return bgp_attr_malformed (&attr_args, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + attr_args.total); /* OK check attribute and store it's value. */ switch (type) { case BGP_ATTR_ORIGIN: - ret = bgp_attr_origin (peer, length, attr, flag, startp); + ret = bgp_attr_origin (&attr_args); break; case BGP_ATTR_AS_PATH: - ret = bgp_attr_aspath (peer, length, attr, flag, startp); + ret = bgp_attr_aspath (&attr_args); break; case BGP_ATTR_AS4_PATH: - ret = bgp_attr_as4_path (peer, length, attr, flag, startp, &as4_path); + ret = bgp_attr_as4_path (&attr_args, &as4_path); break; case BGP_ATTR_NEXT_HOP: - ret = bgp_attr_nexthop (peer, length, attr, flag, startp); + ret = bgp_attr_nexthop (&attr_args); break; case BGP_ATTR_MULTI_EXIT_DISC: - ret = bgp_attr_med (peer, length, attr, flag, startp); + ret = bgp_attr_med (&attr_args); break; case BGP_ATTR_LOCAL_PREF: - ret = bgp_attr_local_pref (peer, length, attr, flag, startp); + ret = bgp_attr_local_pref (&attr_args); break; case BGP_ATTR_ATOMIC_AGGREGATE: - ret = bgp_attr_atomic (peer, length, attr, flag, startp); + ret = bgp_attr_atomic (&attr_args); break; case BGP_ATTR_AGGREGATOR: - ret = bgp_attr_aggregator (peer, length, attr, flag, startp); + ret = bgp_attr_aggregator (&attr_args); break; case BGP_ATTR_AS4_AGGREGATOR: - ret = bgp_attr_as4_aggregator (peer, length, attr, flag, - &as4_aggregator, - &as4_aggregator_addr, startp); + ret = bgp_attr_as4_aggregator (&attr_args, + &as4_aggregator, + &as4_aggregator_addr); break; case BGP_ATTR_COMMUNITIES: - ret = bgp_attr_community (peer, length, attr, flag, startp); + ret = bgp_attr_community (&attr_args); break; case BGP_ATTR_ORIGINATOR_ID: - ret = bgp_attr_originator_id (peer, length, attr, flag, startp); + ret = bgp_attr_originator_id (&attr_args); break; case BGP_ATTR_CLUSTER_LIST: - ret = bgp_attr_cluster_list (peer, length, attr, flag, startp); + ret = bgp_attr_cluster_list (&attr_args); break; case BGP_ATTR_MP_REACH_NLRI: - ret = bgp_mp_reach_parse (peer, length, attr, flag, startp, mp_update); + ret = bgp_mp_reach_parse (&attr_args, mp_update); break; case BGP_ATTR_MP_UNREACH_NLRI: - ret = bgp_mp_unreach_parse (peer, length, flag, startp, mp_withdraw); + ret = bgp_mp_unreach_parse (&attr_args, mp_withdraw); break; case BGP_ATTR_EXT_COMMUNITIES: - ret = bgp_attr_ext_communities (peer, length, attr, flag, startp); + ret = bgp_attr_ext_communities (&attr_args); break; default: - ret = bgp_attr_unknown (peer, attr, flag, type, length, startp); + ret = bgp_attr_unknown (&attr_args); break; } @@ -2020,7 +1941,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, * all attributes first, including these 32bit ones, and now, * afterwards, we look what and if something is to be done for as4. */ - if (bgp_attr_munge_as4_attrs (peer, attr, flag, as4_path, + if (bgp_attr_munge_as4_attrs (peer, attr, as4_path, as4_aggregator, &as4_aggregator_addr)) { if (as4_path) @@ -2052,7 +1973,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, */ if (attr->flag & (ATTR_FLAG_BIT(BGP_ATTR_AS_PATH))) { - ret = bgp_attr_aspath_check (peer, attr, flag); + ret = bgp_attr_aspath_check (peer, attr); if (ret != BGP_ATTR_PARSE_PROCEED) return ret; } -- cgit v1.2.1 From bd471fea4ec965c71d6c2201745995092fbc36f6 Mon Sep 17 00:00:00 2001 From: Paul Jakma Date: Thu, 15 Mar 2012 11:30:00 +0000 Subject: bgpd: malformed attribute error that can still proceed should fixup getp * bgp_attr.c: (bgp_attr_malformed) When a malformed attribute error can be ignored, and BGP message processing may still proceed, the stream getp should be adjusted to the end of the attribute - the caller may not have consumed all the attribute. Problem noted by Martin Winter in bug 678. Also, rename the 'startp' local to 'notify_datap', for clarity. --- bgpd/bgp_attr.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index b02cfee3..d204cec1 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -719,17 +719,24 @@ bgp_attr_malformed (struct bgp_attr_parser_args *args, u_char subcode, * send the attribute data with the NOTIFY depends on the error, * the caller therefore signals this with the seperate length argument */ - u_char *startp = (length > 0 ? args->startp : NULL); + u_char *notify_datap = (length > 0 ? args->startp : NULL); /* Only relax error handling for eBGP peers */ if (peer_sort (peer) != BGP_PEER_EBGP) { bgp_notify_send_with_data (peer, BGP_NOTIFY_UPDATE_ERR, subcode, - startp, length); + notify_datap, length); return BGP_ATTR_PARSE_ERROR; } + /* Adjust the stream getp to the end of the attribute, in case we can + * still proceed but the caller hasn't read all the attribute. + */ + stream_set_getp (BGP_INPUT (peer), + (args->startp - STREAM_DATA (BGP_INPUT (peer))) + + args->total); + switch (args->type) { /* where an optional attribute is inconsequential, e.g. it does not affect * route selection, and can be safely ignored then any such attributes @@ -756,7 +763,7 @@ bgp_attr_malformed (struct bgp_attr_parser_args *args, u_char subcode, case BGP_ATTR_MP_UNREACH_NLRI: case BGP_ATTR_EXT_COMMUNITIES: bgp_notify_send_with_data (peer, BGP_NOTIFY_UPDATE_ERR, subcode, - startp, length); + notify_datap, length); return BGP_ATTR_PARSE_ERROR; } -- cgit v1.2.1 From 683f2b86d89fa356d3d1f7a54b0c269baa38b836 Mon Sep 17 00:00:00 2001 From: Paul Jakma Date: Fri, 23 Mar 2012 14:58:45 +0000 Subject: bgpd: Fix silly mistake in bgp_attr_flag_invalid * bgp_attr.c: (bgp_attr_flag_invalid) flags is meant to be masked off with the mask variable... --- bgpd/bgp_attr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index d204cec1..66704169 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -892,7 +892,7 @@ bgp_attr_flag_invalid (struct bgp_attr_parser_args *args) && CHECK_FLAG (flags, BGP_ATTR_FLAG_TRANS)) SET_FLAG (mask, BGP_ATTR_FLAG_PARTIAL); - if ((flags & ~attr_flags_values[attr_code]) + if ((flags & ~mask) == attr_flags_values[attr_code]) return 0; -- cgit v1.2.1 From fa61e16ddebe10a71d98bcd2eba0f630071ea4a7 Mon Sep 17 00:00:00 2001 From: Paul Jakma Date: Sun, 25 Mar 2012 21:31:47 +0100 Subject: bgpd: attr_parse call to attr_malformed should deal with PROCEED error case * bgpd/bgp_attr.c: (bgp_attr_parse) the invalid flag check call to bgp_attr_malformed is pretty useless if it doesn't actually allow for the PROCEED non-error case. --- bgpd/bgp_attr.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index 66704169..c21655c0 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -738,10 +738,10 @@ bgp_attr_malformed (struct bgp_attr_parser_args *args, u_char subcode, + args->total); switch (args->type) { - /* where an optional attribute is inconsequential, e.g. it does not affect - * route selection, and can be safely ignored then any such attributes - * which are malformed should just be ignored and the route processed as - * normal. + /* where an attribute is relatively inconsequential, e.g. it does not + * affect route selection, and can be safely ignored, then any such + * attributes which are malformed should just be ignored and the route + * processed as normal. */ case BGP_ATTR_AS4_AGGREGATOR: case BGP_ATTR_AGGREGATOR: @@ -749,7 +749,7 @@ bgp_attr_malformed (struct bgp_attr_parser_args *args, u_char subcode, return BGP_ATTR_PARSE_PROCEED; /* Core attributes, particularly ones which may influence route - * selection should always cause session resets + * selection, should always cause session resets */ case BGP_ATTR_ORIGIN: case BGP_ATTR_AS_PATH: @@ -1823,9 +1823,15 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size, Attribute Flags Error. The Data field contains the erroneous attribute (type, length and value). */ if (bgp_attr_flag_invalid (&attr_args)) - return bgp_attr_malformed (&attr_args, - BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, - attr_args.total); + { + bgp_attr_parse_ret_t ret; + ret = bgp_attr_malformed (&attr_args, + BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR, + attr_args.total); + if (ret == BGP_ATTR_PARSE_PROCEED) + continue; + return ret; + } /* OK check attribute and store it's value. */ switch (type) -- cgit v1.2.1 From fa5831e85ae9ba7008fb4b4e2d4c7561ae5cb697 Mon Sep 17 00:00:00 2001 From: Paul Jakma Date: Tue, 27 Mar 2012 11:54:04 +0100 Subject: bgpd: bgp_attr_flags_diagnose shouldn't assert * bgpd/bgp_attr.c: (bgp_attr_flags_diagnose) debug code for error-handling paths probably shouldn't assert, instead it should just log that there was no problem. --- bgpd/bgp_attr.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) (limited to 'bgpd/bgp_attr.c') diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index c21655c0..0d82aba0 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -809,7 +809,14 @@ bgp_attr_flags_diagnose (struct bgp_attr_parser_args *args, attr_flag_str[i].str); seen = 1; } - assert (seen); + if (!seen) + { + zlog (args->peer->log, LOG_DEBUG, + "Strange, %s called for attr %s, but no problem found with flags" + " (real flags 0x%x, desired 0x%x)", + __func__, LOOKUP (attr_str, attr_code), + real_flags, desired_flags); + } } /* Required flags for attributes. EXTLEN will be masked off when testing, -- cgit v1.2.1