From ca3ccd8748434719e4670ce812d1310013fad518 Mon Sep 17 00:00:00 2001 From: David Lamparter Date: Wed, 26 Sep 2012 14:52:39 +0200 Subject: zebra: fix sockaddr_dl length assumptions (BZ#737) Quagga makes bad assumptions about sockaddr_dl (on NetBSD, but possibly on other systems as well). Particularly, sizeof(struct sockaddr_dl) returns a size that does not include the full sdl_data field, leading to not enough data being copied. This breaks IPv6 RAs in particular, as a broken mac address from sockaddr_dl will be included in the packets. From: Matthias-Christian Ott Tested-by: Uwe Toenjes <6bone@6bone.informatik.uni-leipzig.de> [further simplified + more comments] Signed-off-by: David Lamparter --- configure.ac | 1 + lib/if.h | 8 +++++++- lib/zclient.c | 2 +- zebra/kernel_socket.c | 15 ++++++++++++--- zebra/zserv.c | 2 +- 5 files changed, 22 insertions(+), 6 deletions(-) diff --git a/configure.ac b/configure.ac index 92c831ab..937c79ce 100755 --- a/configure.ac +++ b/configure.ac @@ -1469,6 +1469,7 @@ AC_CHECK_TYPES([struct sockaddr, struct sockaddr_in, AC_CHECK_MEMBERS([struct sockaddr.sa_len, struct sockaddr_in.sin_len, struct sockaddr_un.sun_len, struct sockaddr_in6.sin6_scope_id, + struct sockaddr_dl.sdl_len, struct if6_aliasreq.ifra_lifetime, struct nd_opt_adv_interval.nd_opt_ai_type], [], [], QUAGGA_INCLUDES) diff --git a/lib/if.h b/lib/if.h index 841ce51e..2116e12e 100644 --- a/lib/if.h +++ b/lib/if.h @@ -103,7 +103,13 @@ struct interface /* Hardware address. */ #ifdef HAVE_STRUCT_SOCKADDR_DL - struct sockaddr_dl sdl; + union { + /* note that sdl_storage is never accessed, it only exists to make space. + * all actual uses refer to sdl - but use sizeof(sdl_storage)! this fits + * best with C aliasing rules. */ + struct sockaddr_dl sdl; + struct sockaddr_storage sdl_storage; + }; #else unsigned short hw_type; u_char hw_addr[INTERFACE_HWADDR_MAX]; diff --git a/lib/zclient.c b/lib/zclient.c index 61c6f730..d3165962 100644 --- a/lib/zclient.c +++ b/lib/zclient.c @@ -734,7 +734,7 @@ zebra_interface_if_set_value (struct stream *s, struct interface *ifp) ifp->mtu6 = stream_getl (s); ifp->bandwidth = stream_getl (s); #ifdef HAVE_STRUCT_SOCKADDR_DL - stream_get (&ifp->sdl, s, sizeof (ifp->sdl)); + stream_get (&ifp->sdl, s, sizeof (ifp->sdl_storage)); #else ifp->hw_addr_len = stream_getl (s); if (ifp->hw_addr_len) diff --git a/zebra/kernel_socket.c b/zebra/kernel_socket.c index cde36bd0..73fabd4c 100644 --- a/zebra/kernel_socket.c +++ b/zebra/kernel_socket.c @@ -477,13 +477,22 @@ ifm_read (struct if_msghdr *ifm) /* * XXX sockaddr_dl contents can be larger than the structure - * definition, so the user of the stored structure must be - * careful not to read off the end. - * + * definition. There are 2 big families here: + * - BSD has sdl_len + sdl_data[16] + overruns sdl_data + * we MUST use sdl_len here or we'll truncate data. + * - Solaris has no sdl_len, but sdl_data[244] + * presumably, it's not going to run past that, so sizeof() + * is fine here. * a nonzero ifnlen from RTA_NAME_GET() means sdl is valid */ if (ifnlen) + { +#ifdef HAVE_STRUCT_SOCKADDR_DL_SDL_LEN + memcpy (&ifp->sdl, sdl, sdl->sdl_len); +#else memcpy (&ifp->sdl, sdl, sizeof (struct sockaddr_dl)); +#endif /* HAVE_STRUCT_SOCKADDR_DL_SDL_LEN */ + } if_add_update (ifp); } diff --git a/zebra/zserv.c b/zebra/zserv.c index 9e47f23f..cb8dbcb3 100644 --- a/zebra/zserv.c +++ b/zebra/zserv.c @@ -153,7 +153,7 @@ zserv_encode_interface (struct stream *s, struct interface *ifp) stream_putl (s, ifp->mtu6); stream_putl (s, ifp->bandwidth); #ifdef HAVE_STRUCT_SOCKADDR_DL - stream_put (s, &ifp->sdl, sizeof (ifp->sdl)); + stream_put (s, &ifp->sdl, sizeof (ifp->sdl_storage)); #else stream_putl (s, ifp->hw_addr_len); if (ifp->hw_addr_len) -- cgit v1.2.1