diff mbox series

[Openvpn-devel,S] Change in openvpn[master]: Fix unaligned access in macOS/Solaris hwaddr

Message ID 83c5e06d672c765f7d6bd8646088869e8a4e04da-HTML@gerrit.openvpn.net
State Superseded
Headers show
Series [Openvpn-devel,S] Change in openvpn[master]: Fix unaligned access in macOS/Solaris hwaddr | expand

Commit Message

flichtenheld (Code Review) Nov. 21, 2023, 4:06 p.m. UTC 
Attention is currently required from: flichtenheld.

Hello flichtenheld,

I'd like you to do a code review.
Please visit

    http://gerrit.openvpn.net/c/openvpn/+/454?usp=email

to review the following change.


Change subject: Fix unaligned access in macOS/Solaris hwaddr
......................................................................

Fix unaligned access in macOS/Solaris hwaddr

The undefined behaviour USAN clang checker found this.

This fix is a bit messy but so are the original structures.

Change-Id: Ia797c8801fa9a9bc10b6674efde5fdbd7132e4a8
---
M src/openvpn/route.c
1 file changed, 33 insertions(+), 8 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/54/454/1
diff mbox series

Patch

diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index ff64938..ab5bff7 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -3643,7 +3643,6 @@ 
     if (rgi->flags & RGI_IFACE_DEFINED)
     {
         struct ifconf ifc;
-        struct ifreq *ifr;
         const int bufsize = 4096;
         char *buffer;
 
@@ -3668,22 +3667,48 @@ 
 
         for (cp = buffer; cp <= buffer + ifc.ifc_len - sizeof(struct ifreq); )
         {
-            ifr = (struct ifreq *)cp;
+            struct ifreq ifr = { 0 };
+            /* this is not always using an 8 byte alignment that struct ifr
+             * requires */
+            memcpy(&ifr, cp, sizeof(struct ifreq));
 #if defined(TARGET_SOLARIS)
-            const size_t len = sizeof(ifr->ifr_name) + sizeof(ifr->ifr_addr);
+            const size_t len = sizeof(ifr.ifr_name) + sizeof(ifr.ifr_addr);
 #else
-            const size_t len = sizeof(ifr->ifr_name) + max(sizeof(ifr->ifr_addr), ifr->ifr_addr.sa_len);
+            const size_t len = sizeof(ifr.ifr_name) + max(sizeof(ifr.ifr_addr), ifr.ifr_addr.sa_len);
 #endif
 
-            if (!ifr->ifr_addr.sa_family)
+            if (!ifr.ifr_addr.sa_family)
             {
                 break;
             }
-            if (!strncmp(ifr->ifr_name, rgi->iface, IFNAMSIZ))
+            if (!strncmp(ifr.ifr_name, rgi->iface, IFNAMSIZ))
             {
-                if (ifr->ifr_addr.sa_family == AF_LINK)
+                if (ifr.ifr_addr.sa_family == AF_LINK)
                 {
-                    struct sockaddr_dl *sdl = (struct sockaddr_dl *)&ifr->ifr_addr;
+                    /* This is a confusing member access on multiple levels.
+                     *
+                     * struct sockaddr_dl is 20 bytes in size and has
+                     * 12 bytes space for the hw address (6 bytes)
+                     * and Ethernet interface name (max 16 bytes)
+                     *
+                     * So if the interface name is more than 6 byte, it
+                     * extends beyond the struct.
+                     *
+                     * This struct is embedded into ifreq that has
+                     * 16 bytes for a sockaddr and also expects this
+                     * struct to potentially extend beyond the bounds of
+                     * the struct.
+                     *
+                     * Since we only copied 32 bytes from cp to ifr but sdl
+                     * might extend after ifr's end, we  need to copy from
+                     * cp directly to avoid missing out on extra bytes
+                     * behind the struct
+                     */
+                    const size_t sock_dl_len = max_int((int) (sizeof(struct sockaddr_dl)),
+                                                       (int) (ifr.ifr_addr.sa_len));
+
+                    struct sockaddr_dl *sdl = gc_malloc(sock_dl_len, true, &gc);
+                    memcpy(sdl, cp + offsetof(struct ifreq, ifr_addr), sock_dl_len);
                     memcpy(rgi->hwaddr, LLADDR(sdl), 6);
                     rgi->flags |= RGI_HWADDR_DEFINED;
                 }