On Sun, Oct 08, 2023 at 04:14:33AM +0000, Asa Yeamans wrote:
> Hey all,
>
> I recently stumbled across a bug in bgpd where when announcing connected
> routes (i.e. network $AF connected) for IPv6 routes over IPv4 TCP BGP
> connections, bgpd was announcing the IPv6 routes with a next hop of ::1, the
> localhost address.
>
> I traced this down in the bgpd code to get_alternate_addr in session.c
> incorrectly calling sa_cmp.
>
> sa_cmp in util.c compares two sockaddr structures and true (non-zero) if they
> are equal and false (zero) if they are different. However, get_alternate_addr
> treats the sa_cmp call as if it behaved like memcmp (zero if equal, non-zero
> if different). This leads to get_alternate_addr behaving incorrectly.
>
> The fix is to change the comparison (sa_cmp(sa, match->ifa_addr) == 0) from
> == to !=.
>
> After implementing the change and running the patched version locally, I have
> confirmed that it properly selects and reports nexthops when the route AF is
> different from the BGP TCP connection AF.
>
Thanks for the report, this is indeed wrong.
The below diff should fix this. I renamed sa_cmp() to sa_equal() to make
it more obvious that a true return value means the two sa are equal.
--
:wq Claudio
Index: session.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
retrieving revision 1.447
diff -u -p -r1.447 session.c
--- session.c 4 Aug 2023 09:20:12 -0000 1.447
+++ session.c 9 Oct 2023 06:56:05 -0000
@@ -1204,7 +1204,7 @@ session_setup_socket(struct peer *p)
/* compare two sockaddrs by converting them into bgpd_addr */
static int
-sa_cmp(struct sockaddr *a, struct sockaddr *b)
+sa_equal(struct sockaddr *a, struct sockaddr *b)
{
struct bgpd_addr ba, bb;
@@ -1224,7 +1224,7 @@ get_alternate_addr(struct sockaddr *sa,
for (match = ifap; match != NULL; match = match->ifa_next)
if (match->ifa_addr != NULL &&
- sa_cmp(sa, match->ifa_addr) == 0)
+ sa_equal(sa, match->ifa_addr))
break;
if (match == NULL) {
