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

2023-12-30 Thread cron2 (Code Review)
Attention is currently required from: 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 2:

(1 comment)

File src/openvpn/route.c:

http://gerrit.openvpn.net/c/openvpn/+/454/comment/ffdd8937_e4a1e87d :
PS2, Line 3703:  * Since we only copied 32 bytes from cp to 
ifr but sdl
> If ifr_addr. […]
Ah, no, it's not that easy.  The code above does some arithmetics with 
len=...+max(), but then doesn't actually memcpy(..., len) bytes but sizeof(ifr).

Need to think through this again.  But however, the comments need work to be 
understandable for mere mortals :-)



--
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: 2
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: cron2 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: plaisthos 
Gerrit-Comment-Date: Sat, 30 Dec 2023 15:46:55 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: cron2 
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


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

2023-12-30 Thread cron2 (Code Review)
Attention is currently required from: 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 2: Code-Review-1

(5 comments)

Patchset:

PS2:
The ifr memcpy() is well understood and necessary.  The sockaddr_dl memcpy() 
needs at least some better comments, but maybe it's not necessary at all.


File src/openvpn/route.c:

http://gerrit.openvpn.net/c/openvpn/+/454/comment/b7cb53d2_d5e9c975 :
PS2, Line 3677:  * requires */
"need to memcpy() to a strict ifr to force 8-byte alignment required for member 
access"


http://gerrit.openvpn.net/c/openvpn/+/454/comment/3ec0b40a_9dd8ee38 :
PS2, Line 3691:  * struct sockaddr_dl is 20 bytes in size 
and has
On FreeBSD 13 this is

```
struct sockaddr_dl {
u_char  sdl_len;/* Total length of sockaddr */
u_char  sdl_family; /* AF_LINK */
u_short sdl_index;  /* if != 0, system given index for interface */
u_char  sdl_type;   /* interface type */
u_char  sdl_nlen;   /* interface name length, no trailing 0 reqd. */
u_char  sdl_alen;   /* link level address length */
u_char  sdl_slen;   /* link layer selector length */
charsdl_data[46];   /* minimum work area, can be larger;
   contains both if name and ll address */
};
```

so the "20 bytes" comment should be qualified "on some of the BSDs" or so 
(NetBSD indeed has only 20 bytes).  OpenBSD has

```
char  sdl_data[24]; /* minimum work area, can be larger;
   contains both if name and ll address;
   big enough for IFNAMSIZ plus 8byte ll addr */
```


http://gerrit.openvpn.net/c/openvpn/+/454/comment/5b60619c_c908b9eb :
PS2, Line 3693:  * and Ethernet interface name (max 16 
bytes)
the wording here confused me ("if it's hw address and interface name, in this 
order, why would we care about size of interface name?") - I'd swap the order 
around ("..for interface name (max 16 bytes) and hw address (max 6 bytes").


http://gerrit.openvpn.net/c/openvpn/+/454/comment/e6546e64_f9ad06e2 :
PS2, Line 3703:  * Since we only copied 32 bytes from cp to 
ifr but sdl
If ifr_addr.sa_len is >16 (because "long address") the max() above will make us 
copy more than 32 bytes.  So maybe we already have everything we need here, and 
can spare ourselves the double twist?

OTOH, the code below does max(sizeof(), sa_len) - so if sa_len is shorter, we 
might copy too much with the second copy?



--
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: 2
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: cron2 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: plaisthos 
Gerrit-Comment-Date: Sat, 30 Dec 2023 15:03:43 +
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] [S] Change in openvpn[master]: Fix unaligned access in macOS, FreeBSD, Solaris hwaddr

2023-11-22 Thread flichtenheld (Code Review)
Attention is currently required from: plaisthos.

flichtenheld 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 2: Code-Review+2


--
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: 2
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: cron2 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: plaisthos 
Gerrit-Comment-Date: Wed, 22 Nov 2023 16:42:53 +
Gerrit-HasComments: No
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] [S] Change in openvpn[master]: Fix unaligned access in macOS, FreeBSD, Solaris hwaddr

2023-11-22 Thread plaisthos (Code Review)
Attention is currently required from: 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 (#2).


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.

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


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

diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index ff64938..f8cfa53 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -3639,11 +3639,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 +3672,44 @@

 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 */
+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 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, 
);
+memcpy(sdl, cp + offsetof(struct ifreq, ifr_addr), 
sock_dl_len);
 memcpy(rgi->hwaddr, LLADDR(sdl), 6);
 rgi->flags |= RGI_HWADDR_DEFINED;
 }
@@ -3691,6 +3717,7 @@
 cp += len;
 }
 }
+#endif /* if !defined(TARGET_SOLARIS) */

 done:
 if (sockfd >= 0)

--
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: 2
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: cron2 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel