Commit Graph

390 Commits

Author SHA1 Message Date
Donatas Abraitis
0f05e56bed bgpd: Validate Addpath capability flags per AF
Send/Receive:
         This field indicates whether the sender is (a) able to receive
         multiple paths from its peer (value 1), (b) able to send
         multiple paths to its peer (value 2), or (c) both (value 3) for
         the <AFI, SAFI>.

         If any other value is received, then the capability SHOULD be
         treated as not understood and ignored [RFC5492].

Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
2023-12-17 21:25:51 +02:00
Donatas Abraitis
61ff7dddfa bgpd: Do not null-terminate the domainname when receiving FQDN capability
This is already handled above, no need to do here, because we could have an
overrun situation where len > 64 and we do out-of-bound actions.

Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
2023-12-01 10:35:19 +02:00
Donatas Abraitis
c37119df45 bgpd: Ignore handling NLRIs if we received MP_UNREACH_NLRI
If we receive MP_UNREACH_NLRI, we should stop handling remaining NLRIs if
no mandatory path attributes received.

In other words, if MP_UNREACH_NLRI received, the remaining NLRIs should be handled
as a new data, but without mandatory attributes, it's a malformed packet.

In normal case, this MUST not happen at all, but to avoid crashing bgpd, we MUST
handle that.

Reported-by: Iggy Frankovic <iggyfran@amazon.com>
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
2023-10-31 17:22:00 +02:00
Russ White
91c5a471a7
Merge pull request #14651 from opensourcerouting/fix/bgpd_coverity_fqdn_capability
bgpd: Drop unnecessary null-termination for fqdn
2023-10-25 07:24:04 -04:00
Russ White
7cdea4f5d0
Merge pull request #14645 from opensourcerouting/fix/crash_mp_reach_nlri
bgpd: A couple more bgpd crashes on malformed attributes
2023-10-25 07:21:25 -04:00
Donatas Abraitis
0df6464216 bgpd: Drop unnecessary null-termination for fqdn
str[len] is already null terminated before:

```
		if (len > BGP_MAX_HOSTNAME) {
			memcpy(&str, data, BGP_MAX_HOSTNAME);
			str[BGP_MAX_HOSTNAME] = '\0';
		} else if (len) {
			memcpy(&str, data, len);
			str[len] = '\0';
		}
```

CID: 1569357

Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
2023-10-25 07:52:38 +03:00
Donatas Abraitis
b08afc81c6 bgpd: Handle MP_REACH_NLRI malformed packets with session reset
Avoid crashing bgpd.

```
(gdb)
bgp_mp_reach_parse (args=<optimized out>, mp_update=0x7fffffffe140) at bgpd/bgp_attr.c:2341
2341			stream_get(&attr->mp_nexthop_global, s, IPV6_MAX_BYTELEN);
(gdb)
stream_get (dst=0x7fffffffe1ac, s=0x7ffff0006e80, size=16) at lib/stream.c:320
320	{
(gdb)
321		STREAM_VERIFY_SANE(s);
(gdb)
323		if (STREAM_READABLE(s) < size) {
(gdb)
34	  return __builtin___memcpy_chk (__dest, __src, __len, __bos0 (__dest));
(gdb)

Thread 1 "bgpd" received signal SIGSEGV, Segmentation fault.
0x00005555556e37be in route_set_aspath_prepend (rule=0x555555aac0d0, prefix=0x7fffffffe050,
    object=0x7fffffffdb00) at bgpd/bgp_routemap.c:2282
2282		if (path->attr->aspath->refcnt)
(gdb)
```

With the configuration:

```
 neighbor 127.0.0.1 remote-as external
 neighbor 127.0.0.1 passive
 neighbor 127.0.0.1 ebgp-multihop
 neighbor 127.0.0.1 disable-connected-check
 neighbor 127.0.0.1 update-source 127.0.0.2
 neighbor 127.0.0.1 timers 3 90
 neighbor 127.0.0.1 timers connect 1
 address-family ipv4 unicast
  redistribute connected
  neighbor 127.0.0.1 default-originate
  neighbor 127.0.0.1 route-map RM_IN in
 exit-address-family
!
route-map RM_IN permit 10
 set as-path prepend 200
exit
```

Reported-by: Iggy Frankovic <iggyfran@amazon.com>
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
2023-10-24 15:22:52 +03:00
Donatas Abraitis
03ee1cadd5 bgpd: Handle FQDN capability using dynamic capabilities
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
2023-10-20 09:36:32 +03:00
Donatas Abraitis
2c0c11f3e8 bgpd: Handle ORF capability using dynamic capabilities
Add an ability to enable/disable ORF capability dynamically without tearing
down the session.

Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
2023-10-18 16:56:02 +03:00
Russ White
9ff1a8c550
Merge pull request #14528 from opensourcerouting/feature/bgpd_handle_addpath_capability_via_dynamic_capability
bgpd: Handle Addpath capability using dynamic capabilities
2023-10-11 10:16:18 -04:00
Donald Sharp
a4fcdc4e48 Revert "bgpd: accept bgp link-state capability"
This reverts commit 67fe40676e.
2023-10-10 16:45:24 -04:00
Donald Sharp
59c3a49166 Revert "bgpd: store bgp link-state prefixes"
This reverts commit 39a8d354c1.
2023-10-10 16:45:00 -04:00
Donatas Abraitis
05cf9d03b3 bgpd: Handle Addpath capability using dynamic capabilities
Changing Addpath type, and or disabling RX (receiving) flag, we can do this
without tearing down the session, and using dynamic capabilities.

Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
2023-10-03 17:44:19 +03:00
Donatas Abraitis
5e8a8d0ed6 bgpd: Validate maximum length of software version when handling via dynamic caps
We should not allow exceeding the stream's length, and also software version
can't be larger than 64 bytes.

Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
2023-09-29 12:13:43 +03:00
Russ White
8e755a03a3
Merge pull request #12649 from louis-6wind/bgp-link-state
bgpd: add basic support of BGP Link-State RFC7752
2023-09-26 10:07:02 -04:00
Donatas Abraitis
61bd60b984 bgpd: Flush per AFI/SAFI capabilities flags, stale_time for LLGR cap
Clear to defaults if receiving dynamic capability with UNSET action.

Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
2023-09-22 20:50:07 +03:00
Donatas Abraitis
f793136d18 bgpd: Clear graceful-restart per AFI/SAFI capability flags when receiving unset
We flushed the main capability received flag, but missed flushing per AFI/SAFI.

Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
2023-09-22 20:50:06 +03:00
Louis Scalbert
39a8d354c1 bgpd: store bgp link-state prefixes
Add the ability to store link-state prefixes in the BGP table.
Store a raw copy of the BGP link state NLRI TLVs as received in the
packet in 'p.u.prefix_linkstate.ptr'.

Signed-off-by: Olivier Dugeon <olivier.dugeon@orange.com>
Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
2023-09-18 14:57:03 +02:00
Louis Scalbert
67fe40676e bgpd: accept bgp link-state capability
Accept the BGP Link-State AFI/SAFI capability when received from a peer
OPEN message.

Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Signed-off-by: Olivier Dugeon <olivier.dugeon@orange.com>
2023-09-18 14:39:59 +02:00
Donatas Abraitis
7e6ca0742c bgpd: Handle LLGR capability using dynamic capabilities
LLGR stale time is exchanged using OPEN messages. In order to
reduce stal time before doing an actual graceful restart + LLGR, it might be useful
to increase the time, but this is not possible without resetting the session.

With this change, it's possible to send dynamic capability with a new value, and
GR will respect a new reset time value when LLGR kicks in.

Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
2023-09-13 11:30:47 +03:00
Donald Sharp
b2f25e1a17 bgpd: First pass of BGP_EVENT_ADD
Pass through a bunch of BGP_EVENT_ADD's and make
the code use a proper connection instead of a
peer->connection.  There still are a bunch
of places where peer->connection is used and
later commits will probably go through and
clean these up more.

Signed-off-by: Donald Sharp <sharpd@nvidia.com>
2023-09-10 08:31:25 -04:00
Donald Sharp
7094cc7f42 bgpd: bgp_packet pass connection around
Modify all the receive functions to pass around the actual
connection being acted upon.  Modify the collision detection
function to look at the possible two connections.

Signed-off-by: Donald Sharp <sharpd@nvidia.com>
2023-09-10 08:31:25 -04:00
Donald Sharp
d2ba78929f bgpd: bgp_fsm_change_status/BGP_TIMER_ON and BGP_EVENT_ADD
Modify bgp_fsm_change_status to be connection oriented and
also make the BGP_TIMER_ON and BGP_EVENT_ADD macros connection
oriented as well.  Attempt to make peer_xfer_conn a bit more
understandable because, frankly it was/is confusing.

Signed-off-by: Donald Sharp <sharpd@nvidia.com>
2023-09-10 08:31:25 -04:00
Donald Sharp
7b1158b169 bgpd: peer_established should be connection oriented
The peer_established function should be connection oriented.

Signed-off-by: Donald Sharp <sharpd@nvidia.com>
2023-09-10 08:31:25 -04:00
Donald Sharp
1f8274e050 bgpd: bgp_open_send is connection oriented not peer oriented
The bgp_open_send function should use a connection oriented
pointer for it's basis.

Signed-off-by: Donald Sharp <sharpd@nvidia.com>
2023-09-10 08:31:25 -04:00
Donald Sharp
3c7ef0a9c7 bgpd: make bgp_timer_set use a peer_connection instead
The bgp_timer_set function should use a peer_connection pointer
instead.

Signed-off-by: Donald Sharp <sharpd@nvidia.com>
2023-09-10 08:31:25 -04:00
Donald Sharp
3842286ed4 bgpd: bgp_notify_send use peer_connection instead of peer
The bgp_notify_send function should use a peer_connection

Signed-off-by: Donald Sharp <sharpd@nvidia.com>
2023-09-09 16:28:05 -04:00
Donald Sharp
981dd86920 bgpd: move t_generate_updgrp_packets into peer_connection
The t_generate_updgrp_packets event pointer belongs in the
peer_connection pointer.

Signed-off-by: Donald Sharp <sharpd@nvidia.com>
2023-09-09 16:28:05 -04:00
Donald Sharp
e79443fcd8 bgpd: move t_routeadv to peer_connection
The t_routeadv belongs to the peer_connection data structure

Signed-off-by: Donald Sharp <sharpd@nvidia.com>
2023-09-09 16:28:05 -04:00
Donatas Abraitis
14e34520dd bgpd: Print a hostname also for GR logs under dynamic capability
Just to be consistent with other zlog_ stuff for dynamic capabilities.

Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
2023-08-30 17:30:27 +03:00
Donatas Abraitis
7d5873cdc4 bgpd: Make sure we have enough data to read restart time and flags for GR cap
Just a safety check to avoid out of bound reading.

Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
2023-08-30 17:29:11 +03:00
Donatas Abraitis
6cc60e303f bgpd: Handle Graceful-Restart capability with dynamic capability
Graceful-Restart restart time is exchanged using OPEN messages. In order to
reduce restart time before doing an actual graceful restart, it might be useful
to increase the time, but this is not possible without resetting the session.

With this change, it's possible to send dynamic capability with a new value, and
GR will respect a new reset time value.

Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
2023-08-30 17:18:53 +03:00
Donald Sharp
15a5de185c
Merge pull request #14300 from opensourcerouting/fix/set_role_as_undefined_when_capability_unset
bgpd: Unset role when receiving UNSET action for dynamic capability
2023-08-30 09:22:12 -04:00
Donatas Abraitis
1f70ceae0a bgpd: Unset role when receiving UNSET action for dynamic capability
Capability was unset, but forgot to unset the role.

Fixes: 5ad080d37a ("bgpd: Handle Role capability via dynamic capabilities for SET/UNSET properly")

Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
2023-08-30 12:33:16 +03:00
Donatas Abraitis
83ed05c7d3 bgpd: Use zlog_err and not zlog_info when we have an error for dynamic capability
Also change the outputs a bit to be consistent and more detailed.

Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
2023-08-29 22:15:55 +03:00
Donatas Abraitis
5ad080d37a bgpd: Handle Role capability via dynamic capabilities for SET/UNSET properly
It was missed to handle UNSET Role capability using dynamic capabilities.

Also move length check before actually handling Role capability.

Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
2023-08-29 10:10:04 +03:00
Donatas Abraitis
28ccc24d38 bgpd: Do not process NLRIs if the attribute length is zero
```
3  0x00007f423aa42476 in __GI_raise (sig=sig@entry=11) at ../sysdeps/posix/raise.c:26
4  0x00007f423aef9740 in core_handler (signo=11, siginfo=0x7fffc414deb0, context=<optimized out>) at lib/sigevent.c:246
5  <signal handler called>
6  0x0000564dea2fc71e in route_set_aspath_prepend (rule=0x564debd66d50, prefix=0x7fffc414ea30, object=0x7fffc414e400)
    at bgpd/bgp_routemap.c:2258
7  0x00007f423aeec7e0 in route_map_apply_ext (map=<optimized out>, prefix=prefix@entry=0x7fffc414ea30,
    match_object=match_object@entry=0x7fffc414e400, set_object=set_object@entry=0x7fffc414e400, pref=pref@entry=0x0) at lib/routemap.c:2690
8  0x0000564dea2d277e in bgp_input_modifier (peer=peer@entry=0x7f4238f59010, p=p@entry=0x7fffc414ea30, attr=attr@entry=0x7fffc414e770,
    afi=afi@entry=AFI_IP, safi=safi@entry=SAFI_UNICAST, rmap_name=rmap_name@entry=0x0, label=0x0, num_labels=0, dest=0x564debdd5130)
    at bgpd/bgp_route.c:1772
9  0x0000564dea2df762 in bgp_update (peer=peer@entry=0x7f4238f59010, p=p@entry=0x7fffc414ea30, addpath_id=addpath_id@entry=0,
    attr=0x7fffc414eb50, afi=afi@entry=AFI_IP, safi=<optimized out>, safi@entry=SAFI_UNICAST, type=9, sub_type=0, prd=0x0, label=0x0,
    num_labels=0, soft_reconfig=0, evpn=0x0) at bgpd/bgp_route.c:4374
10 0x0000564dea2e2047 in bgp_nlri_parse_ip (peer=0x7f4238f59010, attr=attr@entry=0x7fffc414eb50, packet=0x7fffc414eaf0)
    at bgpd/bgp_route.c:6249
11 0x0000564dea2c5a58 in bgp_nlri_parse (peer=peer@entry=0x7f4238f59010, attr=attr@entry=0x7fffc414eb50,
    packet=packet@entry=0x7fffc414eaf0, mp_withdraw=mp_withdraw@entry=false) at bgpd/bgp_packet.c:339
12 0x0000564dea2c5d66 in bgp_update_receive (peer=peer@entry=0x7f4238f59010, size=size@entry=109) at bgpd/bgp_packet.c:2024
13 0x0000564dea2c901d in bgp_process_packet (thread=<optimized out>) at bgpd/bgp_packet.c:2933
14 0x00007f423af0bf71 in event_call (thread=thread@entry=0x7fffc414ee40) at lib/event.c:1995
15 0x00007f423aebb198 in frr_run (master=0x564deb73c670) at lib/libfrr.c:1213
16 0x0000564dea261b83 in main (argc=<optimized out>, argv=<optimized out>) at bgpd/bgp_main.c:505
```

With the configuration:

```
frr version 9.1-dev-MyOwnFRRVersion
frr defaults traditional
hostname ip-172-31-13-140
log file /tmp/debug.log
log syslog
service integrated-vtysh-config
!
debug bgp keepalives
debug bgp neighbor-events
debug bgp updates in
debug bgp updates out
!
router bgp 100
 bgp router-id 9.9.9.9
 no bgp ebgp-requires-policy
 bgp bestpath aigp
 neighbor 172.31.2.47 remote-as 200
 !
 address-family ipv4 unicast
  neighbor 172.31.2.47 default-originate
  neighbor 172.31.2.47 route-map RM_IN in
 exit-address-family
exit
!
route-map RM_IN permit 10
 set as-path prepend 200
exit
!
```

The issue is that we try to process NLRIs even if the attribute length is 0.

Later bgp_update() will handle route-maps and a crash occurs because all the
attributes are NULL, including aspath, where we dereference.

According to the RFC 4271:

A value of 0 indicates that neither the Network Layer
         Reachability Information field nor the Path Attribute field is
         present in this UPDATE message.

But with a fuzzed UPDATE message this can be faked. I think it's reasonable
to skip processing NLRIs if both update_len and attribute_len are 0.

Reported-by: Iggy Frankovic <iggyfran@amazon.com>
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
2023-08-22 22:52:04 +03:00
Donatas Abraitis
451fb24b17
Merge pull request #8790 from donaldsharp/peer_connection
Peer connection
2023-08-21 20:22:53 +03:00
Donatas Abraitis
9b855a692e bgpd: Don't read the first byte of ORF header if we are ahead of stream
Reported-by: Iggy Frankovic iggyfran@amazon.com
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
2023-08-20 23:22:00 +03:00
Donald Sharp
3e5a31b24e bgpd: Convert struct peer_connection to dynamically allocated
As part of the conversion to a `struct peer_connection` it will
be desirable to have 2 pointers one for when we open a connection
and one for when we receive a connection.  Start this actual
conversion over to this in `struct peer`.  If this sounds confusing
take a look at the bgp state machine for connections and how
it resolves the processing of this router opening -vs- this
router receiving an open.  At some point in time the state
machine decides that we are keeping one of the two connections.

Future commits will allow us to untangle the peer/doppelganger
duality with this abstraction.

Signed-off-by: Donald Sharp <sharpd@nvidia.com>
2023-08-18 09:29:04 -04:00
Donald Sharp
5d52756735 bgpd: Move t_process_packet and t_process_packet_error to connection
The t_process_packet thread events should be managed by the connection.

Signed-off-by: Donald Sharp <sharpd@nvidia.com>
2023-08-18 09:29:04 -04:00
Donald Sharp
e20c23fa5b bgpd: Move status and ostatus to struct peer_connection
The status and ostatus are a function of the `struct peer_connection`
move it into that data structure.

Signed-off-by: Donald Sharp <sharpd@nvidia.com>
2023-08-18 09:29:04 -04:00
Donald Sharp
ccb51e8266 bgpd: Convert bgp_io.c to take struct peer_connection
bgp_io.c is clearly connection oriented so let's convert
it over to using `struct peer_connection`

Signed-off-by: Donald Sharp <sharpd@nvidia.com>
2023-08-18 09:29:04 -04:00
Donald Sharp
1f32eb30d9 bgpd: Start abstraction of struct peer_connection
BGP tracks connections based upon the peer.  But the problem
with this is that the doppelganger structure for it is being
created.  This has introduced a bunch of fragileness in that
the peer exists independently of the connections to it.

The whole point of the doppelganger structure was to allow
BGP to both accept and initiate tcp connections and then
when we get one to a `good` state we collapse into the
appropriate one.  The problem with this is that having
2 peer structures for this creates a situation where
we have to make sure we are configing the `right` one
and also make sure that we collapse the two independent
peer structures into 1 acting peer.  This makes no sense
let's abstract out the peer into having 2 connection
one for incoming connections and one for outgoing connections
then we can easily collapse down without having to do crazy
stuff.  In addition people adding new features don't need
to have to go touch a million places in the code.

This is the start of this abstraction.  In this commit
we'll just pull out the fd and input/output buffers
into a connection data structure.  Future commits
will abstract further.

Signed-off-by: Donald Sharp <sharpd@nvidia.com>
2023-08-18 09:29:04 -04:00
Russ White
a84dee73d1
Merge pull request #14154 from opensourcerouting/feature/bgpd_handle_role_capability_using_dynamic_capability
bgpd: Handle role capability using dynamic capability
2023-08-08 10:47:04 -04:00
Donatas Abraitis
ceea81be77
Merge pull request #14139 from donaldsharp/v6_v4_nexthops
V6 v4 nexthops
2023-08-06 20:11:19 +03:00
Donatas Abraitis
50c5908c9f bgpd: Check if peer is established and dynamic capability-aware
Add this logic inside bgp_capability_send() instead of repeating the whole
logic before calling bgp_capability_send().

Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
2023-08-05 23:02:59 +03:00
Donatas Abraitis
454d37aec2 bgpd: Handle role capability using dynamic capability
When setting local-role for the neighbor, force sending ROLE capability via
dynamic capability if it's enabled.

Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
2023-08-05 22:44:45 +03:00
Donatas Abraitis
f3279abe13 bgpd: Add all other capabilities for dynamic handling (placeholders)
Gonna be covered later with further PRs. Now adding them to avoid compiler
errors due to uncovered switch/cases.

Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
2023-08-03 17:08:33 +03:00
Donatas Abraitis
bf11a9eb25 bgpd: Handle software version capability dynamicaly
We have dynamic capability support, but it handles only MP capability.

With this change, we can enable software version capability dynamicaly, without
resetting the session.

Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
2023-08-03 17:08:33 +03:00