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
> ---
> 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(, 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_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(, cp + offsetof(struct ifreq, ifr_addr),
> sizeof(sdl));
> +memcpy(rgi->hwaddr, LLADDR(), 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,
--
Frank Lichtenheld
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
