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

2023-12-31 Thread cron2 (Code Review)
cron2 has submitted this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/454?usp=email )

Change subject: Fix unaligned access in macOS, FreeBSD, Solaris hwaddr
..

Fix unaligned access in macOS, FreeBSD, Solaris hwaddr

The undefined behaviour USAN clang checker found this.

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

Since the API on Solaris/Illuminos does not return the AF_LINK
sockaddr type we are interested in, there is little value in
fixing the code on that platform to iterate through a list
that does not contain the element we are looking for.

Add includes stddef.h for offsetof and integer.h for max_int.

Change-Id: Ia797c8801fa9a9bc10b6674efde5fdbd7132e4a8
Signed-off-by: Arne Schwabe 
Acked-by: Gert Doering 
Message-Id: <20231231173431.31356-1-g...@greenie.muc.de>
URL: 
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg27885.html
Signed-off-by: Gert Doering 
---
M src/openvpn/route.c
1 file changed, 46 insertions(+), 11 deletions(-)




diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index 6cc112c..0e6667f 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -24,6 +24,7 @@
 /*
  * Support routines for adding/deleting network routes.
  */
+#include 

 #ifdef HAVE_CONFIG_H
 #include "config.h"
@@ -40,6 +41,7 @@
 #include "win32.h"
 #include "options.h"
 #include "networking.h"
+#include "integer.h"

 #include "memdbg.h"

@@ -3639,11 +3641,15 @@
 rgi->flags |= RGI_NETMASK_DEFINED;
 }

+#if !defined(TARGET_SOLARIS)
+/* Illumos/Solaris does not provide AF_LINK entries when calling the
+ * SIOCGIFCONF API, so there is little sense to trying to figure out a
+ * MAC address from an API that does not provide that information */
+
 /* try to read MAC addr associated with interface that owns default 
gateway */
 if (rgi->flags & RGI_IFACE_DEFINED)
 {
 struct ifconf ifc;
-struct ifreq *ifr;
 const int bufsize = 4096;
 char *buffer;

@@ -3668,22 +3674,50 @@

 for (cp = buffer; cp <= buffer + ifc.ifc_len - sizeof(struct ifreq); )
 {
-ifr = (struct ifreq *)cp;
-#if defined(TARGET_SOLARIS)
-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);
-#endif
+struct ifreq ifr = { 0 };
+/* this is not always using an 8 byte alignment that struct ifr
+ * requires. Need to memcpy() to a strict ifr to force 8-byte
+ * alignment required for member access */
+memcpy(, cp, sizeof(struct ifreq));
+const size_t len = sizeof(ifr.ifr_name) + 
max(sizeof(ifr.ifr_addr), ifr.ifr_addr.sa_len);

-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_addr;
+/* This is a confusing member access on multiple levels.
+ *
+ * struct sockaddr_dl is 20 bytes (on macOS and NetBSD,
+ * larger on other BSDs) in size and has
+ * 12 bytes space for the Ethernet interface name
+ * (max 16 bytes) and  hw address (6 bytes)
+ *
+ * So if the interface name is more than 6 byte, the
+ * location of hwaddr 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.
+ *
+ * We only copied 32 bytes (size of ifr at least on macOS
+ * might differ on other platforms again) from cp to ifr.
+ *
+ * But as hwaddr might extend but sdl might extend beyond
+ * ifr's. So we need recalculate how large the actual size
+ * of the embedded dl_sock actually is and then also need
+ * to copy it since it also most likely does not have the
+ * proper alignment required to access 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 = 

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

2023-12-31 Thread cron2 (Code Review)
cron2 has uploaded a new patch set (#4) to the change originally created by 
plaisthos. ( http://gerrit.openvpn.net/c/openvpn/+/454?usp=email )

The following approvals got outdated and were removed:
Code-Review+2 by cron2


Change subject: Fix unaligned access in macOS, FreeBSD, Solaris hwaddr
..

Fix unaligned access in macOS, FreeBSD, Solaris hwaddr

The undefined behaviour USAN clang checker found this.

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

Since the API on Solaris/Illuminos does not return the AF_LINK
sockaddr type we are interested in, there is little value in
fixing the code on that platform to iterate through a list
that does not contain the element we are looking for.

Add includes stddef.h for offsetof and integer.h for max_int.

Change-Id: Ia797c8801fa9a9bc10b6674efde5fdbd7132e4a8
Signed-off-by: Arne Schwabe 
Acked-by: Gert Doering 
Message-Id: <20231231173431.31356-1-g...@greenie.muc.de>
URL: 
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg27885.html
Signed-off-by: Gert Doering 
---
M src/openvpn/route.c
1 file changed, 46 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/54/454/4

diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index 6cc112c..0e6667f 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -24,6 +24,7 @@
 /*
  * Support routines for adding/deleting network routes.
  */
+#include 

 #ifdef HAVE_CONFIG_H
 #include "config.h"
@@ -40,6 +41,7 @@
 #include "win32.h"
 #include "options.h"
 #include "networking.h"
+#include "integer.h"

 #include "memdbg.h"

@@ -3639,11 +3641,15 @@
 rgi->flags |= RGI_NETMASK_DEFINED;
 }

+#if !defined(TARGET_SOLARIS)
+/* Illumos/Solaris does not provide AF_LINK entries when calling the
+ * SIOCGIFCONF API, so there is little sense to trying to figure out a
+ * MAC address from an API that does not provide that information */
+
 /* try to read MAC addr associated with interface that owns default 
gateway */
 if (rgi->flags & RGI_IFACE_DEFINED)
 {
 struct ifconf ifc;
-struct ifreq *ifr;
 const int bufsize = 4096;
 char *buffer;

@@ -3668,22 +3674,50 @@

 for (cp = buffer; cp <= buffer + ifc.ifc_len - sizeof(struct ifreq); )
 {
-ifr = (struct ifreq *)cp;
-#if defined(TARGET_SOLARIS)
-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);
-#endif
+struct ifreq ifr = { 0 };
+/* this is not always using an 8 byte alignment that struct ifr
+ * requires. Need to memcpy() to a strict ifr to force 8-byte
+ * alignment required for member access */
+memcpy(, cp, sizeof(struct ifreq));
+const size_t len = sizeof(ifr.ifr_name) + 
max(sizeof(ifr.ifr_addr), ifr.ifr_addr.sa_len);

-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_addr;
+/* This is a confusing member access on multiple levels.
+ *
+ * struct sockaddr_dl is 20 bytes (on macOS and NetBSD,
+ * larger on other BSDs) in size and has
+ * 12 bytes space for the Ethernet interface name
+ * (max 16 bytes) and  hw address (6 bytes)
+ *
+ * So if the interface name is more than 6 byte, the
+ * location of hwaddr 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.
+ *
+ * We only copied 32 bytes (size of ifr at least on macOS
+ * might differ on other platforms again) from cp to ifr.
+ *
+ * But as hwaddr might extend but sdl might extend beyond
+ * ifr's. So we need recalculate how large the actual size
+ * of the embedded dl_sock actually is and then also need
+ * to copy it since it also most likely does not have the
+ * proper alignment required to access the struct.
+ */
+

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

2023-12-31 Thread cron2 (Code Review)
Attention is currently required from: flichtenheld, plaisthos.

cron2 has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/454?usp=email )

Change subject: Fix unaligned access in macOS, FreeBSD, Solaris hwaddr
..


Patch Set 3: Code-Review+2

(1 comment)

Patchset:

PS3:
I still find this all horribly ugly, but this is not your fault... this API is 
bad enough (... on MacOS, it seems) but combined with C "undefined behaviour" 
rules there is no hope for sanity...

That said, I have tested this on FreeBSD (works), NetBSD (works), OpenBSD (no 
AF_LINK provided, so, didn't work before, doesn't break anything now), MacOS 
(works) and OpenSolaris (no AF_LINK provided, but it compiles and tests fine).



--
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: 3
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: cron2 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: plaisthos 
Gerrit-Attention: flichtenheld 
Gerrit-Comment-Date: Sun, 31 Dec 2023 17:33:53 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


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

2023-12-31 Thread plaisthos (Code Review)
Attention is currently required from: cron2, flichtenheld, plaisthos.

Hello cron2, flichtenheld,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#3).

The following approvals got outdated and were removed:
Code-Review+2 by flichtenheld, Code-Review-1 by cron2

The change is no longer submittable: Code-Review and checks~ChecksSubmitRule 
are unsatisfied now.


Change subject: Fix unaligned access in macOS, FreeBSD, Solaris hwaddr
..

Fix unaligned access in macOS, FreeBSD, Solaris hwaddr

The undefined behaviour USAN clang checker found this.

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

Since the API on Solaris/Illuminos does not return the AF_LINK
sockaddr type we are interested in, there is little value in
fixing the code on that platform to iterate through a list
that does not contain the element we are looking for.

Add includes stddef.h for offsetof and integer.h for max_int.

Change-Id: Ia797c8801fa9a9bc10b6674efde5fdbd7132e4a8
Signed-off-by: Arne Schwabe 
---
M src/openvpn/route.c
1 file changed, 46 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/54/454/3

diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index 6cc112c..0e6667f 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -24,6 +24,7 @@
 /*
  * Support routines for adding/deleting network routes.
  */
+#include 

 #ifdef HAVE_CONFIG_H
 #include "config.h"
@@ -40,6 +41,7 @@
 #include "win32.h"
 #include "options.h"
 #include "networking.h"
+#include "integer.h"

 #include "memdbg.h"

@@ -3639,11 +3641,15 @@
 rgi->flags |= RGI_NETMASK_DEFINED;
 }

+#if !defined(TARGET_SOLARIS)
+/* Illumos/Solaris does not provide AF_LINK entries when calling the
+ * SIOCGIFCONF API, so there is little sense to trying to figure out a
+ * MAC address from an API that does not provide that information */
+
 /* try to read MAC addr associated with interface that owns default 
gateway */
 if (rgi->flags & RGI_IFACE_DEFINED)
 {
 struct ifconf ifc;
-struct ifreq *ifr;
 const int bufsize = 4096;
 char *buffer;

@@ -3668,22 +3674,50 @@

 for (cp = buffer; cp <= buffer + ifc.ifc_len - sizeof(struct ifreq); )
 {
-ifr = (struct ifreq *)cp;
-#if defined(TARGET_SOLARIS)
-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);
-#endif
+struct ifreq ifr = { 0 };
+/* this is not always using an 8 byte alignment that struct ifr
+ * requires. Need to memcpy() to a strict ifr to force 8-byte
+ * alignment required for member access */
+memcpy(, cp, sizeof(struct ifreq));
+const size_t len = sizeof(ifr.ifr_name) + 
max(sizeof(ifr.ifr_addr), ifr.ifr_addr.sa_len);

-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_addr;
+/* This is a confusing member access on multiple levels.
+ *
+ * struct sockaddr_dl is 20 bytes (on macOS and NetBSD,
+ * larger on other BSDs) in size and has
+ * 12 bytes space for the Ethernet interface name
+ * (max 16 bytes) and  hw address (6 bytes)
+ *
+ * So if the interface name is more than 6 byte, the
+ * location of hwaddr 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.
+ *
+ * We only copied 32 bytes (size of ifr at least on macOS
+ * might differ on other platforms again) from cp to ifr.
+ *
+ * But as hwaddr might extend but sdl might extend beyond
+ * ifr's. So we need recalculate how large the actual size
+ * of the embedded dl_sock actually is and then also need
+ * to copy it since it also most likely does not have the
+ * proper alignment required to access the struct.
+