On Mon, Oct 09, 2023 at 08:58:25AM +0200, Claudio Jeker wrote:
> 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.
Makes sense. You could unwrap the line if you want.
ok tb
>
> --
> :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) {
