| Message ID | 20230130172936.3444840-2-arne@rfc2549.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel,1/5] Fix unaligned access in auth-token | expand |
On Mon, Jan 30, 2023 at 06:29:33PM +0100, Arne Schwabe wrote: > The undefined behaviour USAN clang checker found this. > > This fix is a bit messy but so are the original structures. > Acked-By: Frank Lichtenheld <frank@lichtenheld.com> Well, it doesn't make it worse vOv Regards,
Hi, On Mon, Jan 30, 2023 at 06:29:33PM +0100, Arne Schwabe wrote: > The undefined behaviour USAN clang checker found this. > > This fix is a bit messy but so are the original structures. I wonder... > + struct sockaddr_dl sdl = { 0 }; > + memcpy(&sdl, &ifr.ifr_addr, sizeof(ifr.ifr_addr)); > + memcpy(rgi->hwaddr, LLADDR(&sdl), 6); > rgi->flags |= RGI_HWADDR_DEFINED; ... why you keep the "sockaddr_dl" intermediate - this was useful for "we use it as a pointer", but doing two memcpy() with a huge explanation above looks weird. LLADDR on FreeBSD is #define LLADDR(s) ((caddr_t)((s)->sdl_data + (s)->sdl_nlen)) (to operate on a "struct sockaddr_dl") struct ifaddr has struct ifaddr { struct sockaddr *ifa_addr; /* address of interface */ struct sockaddr *ifa_dstaddr; /* other end of p-to-p link */ struct sockaddr *ifa_netmask; /* used to determine subnet */ ... so the first memcpy() isn't actually copying the MAC address around, just a bunch of pointers, and then we dereference the first of them for the second memcpy(), using the dereferencing macro for a "sockaddr_dl" structure. FreeBSD also has #define IF_LLADDR(ifp) \ LLADDR((struct sockaddr_dl *)((ifp)->if_addr->ifa_addr)) which sounds like "feed in an *ifr, out comes a pointer for the memcpy() to rgi->addr". I'll check the other BSDs if LL_IFADDR() is available everywhere, but if not, we can just do the same thing (struct ifr is aligned) and get rid of the "sdl" and the first memcpy(). (Am I overlooking something?) gert
Hi, On Sun, Feb 26, 2023 at 11:02:16AM +0100, Gert Doering wrote: > I'll check the other BSDs if LL_IFADDR() is available everywhere, but > if not, we can just do the same thing (struct ifr is aligned) and > get rid of the "sdl" and the first memcpy(). Of course neither Solaris nor OpenBSD have the LL_IFADDR(), but that's trivial enough. gert
Am 26.02.2023 um 11:02 schrieb Gert Doering: > Hi, > > On Mon, Jan 30, 2023 at 06:29:33PM +0100, Arne Schwabe wrote: >> The undefined behaviour USAN clang checker found this. >> >> This fix is a bit messy but so are the original structures. > I wonder... > >> + struct sockaddr_dl sdl = { 0 }; >> + memcpy(&sdl, &ifr.ifr_addr, sizeof(ifr.ifr_addr)); >> + memcpy(rgi->hwaddr, LLADDR(&sdl), 6); >> rgi->flags |= RGI_HWADDR_DEFINED; > ... why you keep the "sockaddr_dl" intermediate - this was useful > for "we use it as a pointer", but doing two memcpy() with a huge > explanation above looks weird. > > LLADDR on FreeBSD is > > #define LLADDR(s) ((caddr_t)((s)->sdl_data + (s)->sdl_nlen)) > > (to operate on a "struct sockaddr_dl") > > struct ifaddr has > > struct ifaddr { > struct sockaddr *ifa_addr; /* address of interface */ > struct sockaddr *ifa_dstaddr; /* other end of p-to-p link */ > struct sockaddr *ifa_netmask; /* used to determine subnet */ > > ... so the first memcpy() isn't actually copying the MAC address around, > just a bunch of pointers, and then we dereference the first of them > for the second memcpy(), using the dereferencing macro for a > "sockaddr_dl" structure. The problem is that LLVM ASAN does not like access to an unaligned struct. Even if it just accessing a member to deference it. So you are right that it is just copying a bunch of pointers, to not triggers the warning, we have to do this nonsense. The compiler will even optimise this then away again on platforms that have no problem with unaligned accesses. > FreeBSD also has > > #define IF_LLADDR(ifp) \ > LLADDR((struct sockaddr_dl *)((ifp)->if_addr->ifa_addr)) > > which sounds like "feed in an *ifr, out comes a pointer for the > memcpy() to rgi->addr". Yes but you will still get warning about casting a smaller struct to a larger struct and that being a bad idea. Arne
Hi, On Sun, Feb 26, 2023 at 03:10:07PM +0100, Arne Schwabe wrote: > > struct ifaddr { > > struct sockaddr *ifa_addr; /* address of interface */ > > struct sockaddr *ifa_dstaddr; /* other end of p-to-p link */ > > struct sockaddr *ifa_netmask; /* used to determine subnet */ > > > > ... so the first memcpy() isn't actually copying the MAC address around, > > just a bunch of pointers, and then we dereference the first of them > > for the second memcpy(), using the dereferencing macro for a > > "sockaddr_dl" structure. > > The problem is that LLVM ASAN does not like access to an unaligned > struct. Even if it just accessing a member to deference it. So you are > right that it is just copying a bunch of pointers, to not triggers the > warning, we have to do this nonsense. The compiler will even optimise > this then away again on platforms that have no problem with unaligned > accesses. With ifr being a structure now, it is aligned just fine - I'm not objecting to that part, just to the intermediate "sdl" thing, and the double memcpy(), and 10 lines of comments. > > FreeBSD also has > > > > #define IF_LLADDR(ifp) \ > > LLADDR((struct sockaddr_dl *)((ifp)->if_addr->ifa_addr)) > > > > which sounds like "feed in an *ifr, out comes a pointer for the > > memcpy() to rgi->addr". > > Yes but you will still get warning about casting a smaller struct to a > larger struct and that being a bad idea. This is not casting a struct, just casting a pointer. Does it still warn? gert
On Tue, Jan 31, 2023 at 11:59:41AM +0100, Frank Lichtenheld wrote: > On Mon, Jan 30, 2023 at 06:29:33PM +0100, Arne Schwabe wrote: > > The undefined behaviour USAN clang checker found this. > > > > This fix is a bit messy but so are the original structures. > > > > Acked-By: Frank Lichtenheld <frank@lichtenheld.com> > > Well, it doesn't make it worse vOv Note: I revoke this ACK. Would need to verify first whether we need further changes similar to what Arne did in OpenVPN3: https://github.com/OpenVPN/openvpn3/commit/b755783a13257237f85d633fe2ae60e4118415a0 Regards,
diff --git a/src/openvpn/route.c b/src/openvpn/route.c index 82519c94b..06bfb799c 100644 --- a/src/openvpn/route.c +++ b/src/openvpn/route.c @@ -3637,7 +3637,7 @@ get_default_gateway(struct route_gateway_info *rgi, openvpn_net_ctx_t *ctx) if (rgi->flags & RGI_IFACE_DEFINED) { struct ifconf ifc; - struct ifreq *ifr; + struct ifreq ifr; const int bufsize = 4096; char *buffer; @@ -3662,23 +3662,37 @@ get_default_gateway(struct route_gateway_info *rgi, openvpn_net_ctx_t *ctx) for (cp = buffer; cp <= buffer + ifc.ifc_len - sizeof(struct ifreq); ) { - ifr = (struct ifreq *)cp; + /* this is not always using an 8byte 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; - memcpy(rgi->hwaddr, LLADDR(sdl), 6); + /* This is a broken member access. struct sockaddr_dl has + * 20 bytes while if_addr has only 16 bytes. So casting if_addr + * to struct sockaddr_dl gives (legitimate) warnings + * + * sockaddr_dl has 12 bytes space for the hw address and + * Ethernet only uses 6 bytes. So the last 4 that are + * truncated and not in if_addr can be ignored here. + * + * So we use a memcpy here to avoid the warnings with ASAN + * that we are doing a very nasty cast here + */ + struct sockaddr_dl sdl = { 0 }; + memcpy(&sdl, &ifr.ifr_addr, sizeof(ifr.ifr_addr)); + memcpy(rgi->hwaddr, LLADDR(&sdl), 6); rgi->flags |= RGI_HWADDR_DEFINED; } }
The undefined behaviour USAN clang checker found this. This fix is a bit messy but so are the original structures. Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- src/openvpn/route.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-)