zebra: kernel_socket: fix overflow in RTA_ADDR & RTA_ATTR

In zebra/kernel_socket.c, copying sockaddr from *_msghdr:

There are really 2 different lengths that need to be determined.
  1) the length required to point to the next sockaddr in the mesg
     buffer which might include any required padding and
  2) the actual length of the sockaddr data that needs to be copied
     into the destination field.
They may or may not be the same value.

Sizeof sockaddr_in6 is 28, which to pad for alignment purposes on 32
bit systems with a long of 4 bytes is evenly divided and requires
no padding. On 64 bit systems, with a long of 8 it is padded with 4
extra bytes.So the current RTA_* macros are copying 32 bytes into a 28
byte field on 64 bitsystems, where the field overflow did not occur
on the 32 bit systems.

Since using sa_len required the use of an #ifdef which couldn't be used
directly inside a #define, it made sense to move the copy into the
function to allow typdef checking throughout and eliminate the hack
to suppress compiler warnings.

Fixed declaration of cp in ifm_read after compiler noticed type mismatch.

Tested on 64bit OS X 10.7, FreeBSD 9.0 amd64 & i386 (32bit)
using gcc & clang

Signed-off-by: David Lamparter <equinox@opensourcerouting.org>

1 files changed, 22 insertions, 9 deletions

diff --git a/zebra/kernel_socket.c b/zebra/kernel_socket.c
index ecc6e790..20c17f9e 100644
--- a/zebra/kernel_socket.c
+++ b/zebra/kernel_socket.c
@@ -84,28 +84,41 @@ extern struct zebra_t zebrad;
            ROUNDUP(sizeof(struct sockaddr_dl)) : sizeof(struct sockaddr)))
 #endif /* HAVE_STRUCT_SOCKADDR_SA_LEN */
 
-/* We use an additional pointer in following, pdest, rather than (DEST)
- * directly, because gcc will warn if the macro is expanded and DEST is NULL,
- * complaining that memcpy is being passed a NULL value, despite the fact
- * the if (NULL) makes it impossible.
+/*
+ * We use a call to an inline function to copy (PNT) to (DEST)
+ * 1. Calculating the length of the copy requires an #ifdef to determine
+ *    if sa_len is a field and can't be used directly inside a #define
+ * 2. So the compiler doesn't complain when DEST is NULL, which is only true
+ *    when we are skipping the copy and incrementing to the next SA
  */
+static void inline
+rta_copy (union sockunion *dest, caddr_t src) {
+  int len;
+#ifdef HAVE_STRUCT_SOCKADDR_SA_LEN
+  len = (((struct sockaddr *)src)->sa_len > sizeof (*dest)) ?
+            sizeof (*dest) : ((struct sockaddr *)src)->sa_len ;
+#else
+  len = (SAROUNDUP (src) > sizeof (*dest)) ?
+            sizeof (*dest) : SAROUNDUP (src) ;
+#endif
+  memcpy (dest, src, len);
+}
+
 #define RTA_ADDR_GET(DEST, RTA, RTMADDRS, PNT) \
   if ((RTMADDRS) & (RTA)) \
     { \
-      void *pdest = (DEST); \
       int len = SAROUNDUP ((PNT)); \
       if ( ((DEST) != NULL) && \
            af_check (((struct sockaddr *)(PNT))->sa_family)) \
-        memcpy (pdest, (PNT), len); \
+        rta_copy((DEST), (PNT)); \
       (PNT) += len; \
     }
 #define RTA_ATTR_GET(DEST, RTA, RTMADDRS, PNT) \
   if ((RTMADDRS) & (RTA)) \
     { \
-      void *pdest = (DEST); \
       int len = SAROUNDUP ((PNT)); \
       if ((DEST) != NULL) \
-        memcpy (pdest, (PNT), len); \
+        rta_copy((DEST), (PNT)); \
       (PNT) += len; \
     }
 
@@ -328,7 +341,7 @@ ifm_read (struct if_msghdr *ifm)
   struct sockaddr_dl *sdl;
   char ifname[IFNAMSIZ];
   short ifnlen = 0;
-  caddr_t *cp;
+  caddr_t cp;
   
   /* terminate ifname at head (for strnlen) and tail (for safety) */
   ifname[IFNAMSIZ - 1] = '\0';