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) {