Browse Source

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 <ott@mirix.org>
Tested-by: Uwe Toenjes <6bone@6bone.informatik.uni-leipzig.de>
[further simplified + more comments]
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
David Lamparter 7 years ago
parent
commit
ca3ccd8748
5 changed files with 22 additions and 6 deletions
  1. 1 0
      configure.ac
  2. 7 1
      lib/if.h
  3. 1 1
      lib/zclient.c
  4. 12 3
      zebra/kernel_socket.c
  5. 1 1
      zebra/zserv.c

+ 1 - 0
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)

+ 7 - 1
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];

+ 1 - 1
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)

+ 12 - 3
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);
     }

+ 1 - 1
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)