christos@astron.com (Christos Zoulas) writes:

[dropping current-users; we're polishing now]


> In article <rmify1i2zcn.fsf@fnord.ir.bbn.com>,
> Greg Troxel  <gdt@ir.bbn.com> wrote:
>>I looked at the ifconf() code and now think that the kernel wasn't
>>copying the whole sockaddr.  I have compile tested but not run the
>>following patch, which I think will fix the 'missing two bytes' of the
>>mac address problem, as well as unbreak v6 addresses.
>>
>>Index: sys/net/if.c
>>===================================================================
>>RCS file: /cvsroot/src/sys/net/if.c,v
>>retrieving revision 1.200
>>diff -u -p -r1.200 if.c
>>--- sys/net/if.c	11 Sep 2007 19:31:22 -0000	1.200
>>+++ sys/net/if.c	13 Sep 2007 18:28:48 -0000
>>@@ -1669,7 +1669,7 @@ ifconf(u_long cmd, void *data)
>> 
>> 			if (ifrp != NULL)
>> 			{
>>-				ifr.ifr_addr = *sa;
>>+				memcpy(&ifr.ifr_space, sa, sa->sa_len);
>
> I think that this should be MIN(sa->sa_len, sizeof(ifr.ifr_space)) just
> to be paranoid... Or even better:
>
> _KASSERT(sa->sa_len < sizeof(ifr.ifr_space));
>
> before the memcpy...
>
> christos


I think so too, but the whole loop has:

		TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
			struct sockaddr *sa = ifa->ifa_addr;
			/* all sockaddrs must fit in sockaddr_storage */
			KASSERT(sa->sa_len <= sizeof(ifr.ifr_ifru));

			if (ifrp != NULL)
			{
				memcpy(&ifr.ifr_space, sa, sa->sa_len);
				if (space >= sz) {
					error = copyout(&ifr, ifrp, sz);
					if (error != 0)
						return (error);
					ifrp++; space -= sz;
				}
			}
			else
				space += sz;
		}

So perhaps you could argue that the KASSERT only belongs in the if branch.

I used the union, not space, because that's the real destination of the
copy.  But they should be the same.

Why < vs <= ?

Is _KASSERT just a typeo for KASSERT?