Re: memcpy overlap in getinterfaces(int*) (tcpip.cc:2987)

[David Fifield <david () bamsoftware com>]

On Mon, Jun 29, 2009 at 02:25:31PM -0600, David Fifield wrote:
> On Tue, Jun 16, 2009 at 10:07:23PM +0000, Brandon Enright wrote:
> > I was recently testing Nmap in Valgrind and noticed that I always get
> > the following warning:
> >
> > ==20569== Source and destination overlap in memcpy(0x7FEFFA160, 0x7FEFFA160, 16)
> > ==20569==    at 0x4C2590A: memcpy (in /usr/lib64/valgrind/amd64-linux/vgpreload_memcheck.so)
> >
> > Valgrind is (rightfully) complaining that the source and destination
> > address are the same to a call to memcpy().  In looking at the code, it
> > seems that they will often be the same:
> >
> > Starting at tcpip.cc:2977
> >
> >         sin = (struct sockaddr_in *) &(tmpifr.ifr_addr); /* ifr_netmask only on Linux */
> >         addr_stob(&(tmpifr.ifr_addr), &mydevs[numifaces].netmask_bits);
> >       }
> >
> > ...comments...
> >
> >       Strncpy(tmpifr.ifr_name, ifr->ifr_name, sizeof(tmpifr.ifr_name));
> >       memcpy(&(tmpifr.ifr_addr), sin, MIN(sizeof(tmpifr.ifr_addr), sizeof(*sin)\));
> >
> > Does anybody have any ideas about the right way to fix this code?
> > Should we just drop the call to memcpy?
>
> The two lines you cited can be removed because they always copy
> something that's already there. There's another similar call to memcpy
> down below that I think can also be removed.

I think I have this figured out, and fixed in r14118. My previous
suggestion to remove the lines was wrong but probably harmless.

What's going on with all those Strncpy and memcpy? All that is done
because it's part of the interface of the SIOCG* ioctls used in the
getinterfaces function. Each of those takes a pointer to an ifreq
structure, which has an ifr_name plus a union containing other members
like ifr_addr and ifr_flags. On Linux, netdevice(7) says

        Normally, the user specifies which device to affect by setting
        ifr_name to the name of the interface. All other members of the
        structure may share memory.

So the way you are expected to use the ioctls is to first copy in the
ifr_name, then call ioctl and read the result from the same structure:

        Strncpy(tmpifr.ifr_name, ifr->ifr_name, sizeof(tmpifr.ifr_name));
        rc = ioctl(sd, SIOCGIFFLAGS, &tmpifr);
        /* Now examine tmpifr.ifr_flags, which will have overwritten
           anything else in tmpifr, except for ifr_name. */

Because ifr_name is not overwritten, you only need to set it once; all
the others can indeed be removed. But what about the memcpy of ifr_addr?
My best guess is that this is required on some non-Linux platforms to
identify the interface because ifr_name doesn't work on its own. The
memcpy lines were added in r2762, which has the log message

        Have it compiling on Solaris, and soon should have it working.

I also found a link saying that Cygwin uses ifr_addr, but the same link
says that it's not required on Solaris.

        http://cygwin.com/ml/cygwin/2002-10/msg00884.html

So I don't know if setting ifr_addr is really required or not, but it at
least does no harm. However, because it is overwritten by the ioctl, you
have to reset it each time. That is what the sin pointer is used for. It
stores the address of the interface's IP address so it can be copied
into tmpifr.ifr_addr before each ioctl. In other words, it's not
supposed to change.

The problem was that the code that handles SIOCGIFNETMASK used sin as a
throwaway pointer, without considering that it was still going to be
used by following code. It got redirected to point to &tmpifr.ifr_addr,
so future memcpys copied one thing to itself. My guess as to why this
didn't cause any problems is that ifr_addr is not actually used by the
ioctl on many platforms (apart from Cygwin apparently).

The use of sin was a throwaway pointer was vestigial anyway. The code
that used it was changed in r2751:

        sin = (struct sockaddr_in *) &(tmpifr.ifr_addr); /* ifr_netmask only on Linux */
-       memcpy(&(mydevs[numinterfaces].netmask), (char *) &(sin->sin_addr), sizeof(struct in_addr));
+       addr_stob(&(tmpifr.ifr_addr), &mydevs[numifaces].netmask_bits);

So I just removed the assignment to sin.

Side note: The comment /* ifr_netmask only on Linux */ made it sound as
if the SIOCGIFNETMASK ioctl only worked on Linux or something. I don't
think that's true. What the comment really means is that the ifr_netmask
union member is only defined on Linux. So instead of using that name, we
use the name ifr_addr, which shares the same memory in the union anyway.

Does anyone know of a system that requires ifr_addr to be set for these
ioctls to work?

David Fifield

_______________________________________________
Sent through the nmap-dev mailing list
http://cgi.insecure.org/mailman/listinfo/nmap-dev
Archived at http://SecLists.Org