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 --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;
}
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/454?usp=email
To unsubscribe, or for help writing mail filters, visit
http://gerrit.openvpn.net/settings
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Ia797c8801fa9a9bc10b6674efde5fdbd7132e4a8
Gerrit-Change-Number: 454
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos <[email protected]>
Gerrit-Reviewer: flichtenheld <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: flichtenheld <[email protected]>
Gerrit-MessageType: newchange
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
