| Message ID | 20230810140210.4047842-1-arne@rfc2549.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel,v2] Fix unaligned access in macOS/Solaris hwaddr | expand |
On Thu, Aug 10, 2023 at 04:02:10PM +0200, Arne Schwabe wrote: > The undefined behaviour USAN clang checker found this. > > This fix is a bit messy but so are the original structures. > > Patch v2: handle the fact we need to beyond the struct ifr > correctly when mapping the result to struct sockaddr_dl > > Change-Id: Ia797c8801fa9a9bc10b6674efde5fdbd7132e4a8 > Signed-off-by: Arne Schwabe <arne@rfc2549.org> > --- > src/openvpn/route.c | 32 +++++++++++++++++++++++--------- > 1 file changed, 23 insertions(+), 9 deletions(-) > > diff --git a/src/openvpn/route.c b/src/openvpn/route.c > index 90e981e97..bcf6fb878 100644 > --- a/src/openvpn/route.c > +++ b/src/openvpn/route.c > @@ -3641,7 +3641,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; > > @@ -3666,23 +3666,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 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; > - 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 inrerface name and "interface" > + * the hw address. So the last 4 that might be part of the > + * hw address are not in if_addr, so we need Sentence missing words at the end? > + * > + * 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, cp + offsetof(struct ifreq, ifr_addr), sizeof(sdl)); > + memcpy(rgi->hwaddr, LLADDR(&sdl), 6); > rgi->flags |= RGI_HWADDR_DEFINED; So, reading the documentation about sockaddr and sockaddr_dl, I'm not sure that even this code is safe. E.g. you say "sockaddr_dl has 12 bytes space", but is that actually true? The actual headers always seems to suggest "at least 12 bytes space". Similar to how the generic sockaddr has "at least 16 bytes size". So is that memcpy that you're doing actually guaranteed to copy enough data to make LLADDR work? Or can LLADDR actually point behind sizeof(sockaddr_dl)? The documentation and comments in the BSD headers seem to be vague about that? Regards,
diff --git a/src/openvpn/route.c b/src/openvpn/route.c index 90e981e97..bcf6fb878 100644 --- a/src/openvpn/route.c +++ b/src/openvpn/route.c @@ -3641,7 +3641,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; @@ -3666,23 +3666,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 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; - 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 inrerface name and + * the hw address. So the last 4 that might be part of the + * hw address are not in if_addr, so we need + * + * 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, cp + offsetof(struct ifreq, ifr_addr), sizeof(sdl)); + 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. Patch v2: handle the fact we need to beyond the struct ifr correctly when mapping the result to struct sockaddr_dl Change-Id: Ia797c8801fa9a9bc10b6674efde5fdbd7132e4a8 Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- src/openvpn/route.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-)