In article <20070530005638.GE25295@che.ojctech.com>,
David Young  <dyoung@pobox.com> wrote:
>On Tue, May 29, 2007 at 07:14:21PM -0400, Thor Lancelot Simon wrote:
>> On Tue, May 29, 2007 at 05:20:33PM -0500, David Young wrote:
>> > 
>> > This seems like an awful lot of #ifdef'age to achieve very limited
>> > protection against stack smashing.  Suppose the kernel copies to ifreq
>> > a sockaddr whose sa_len > sizeof(struct sockaddr_storage) ?
>> 
>> The kernel won't: sockaddr_storage is, by definition, large enough to
>> contain any protocol-specific sockaddr.  That's what it's for.
>> 
>> The issue with kernel->user copies was truncation of addresses.  The
>> stack-smashing issue involved legitimate programming practices like
>> trying to zero the entire sockaddr_dl "contained" in an ifreq...
>
>I am not arguing that the change does not offer any protection, but that
>it leaves a big, muddy footprint on the code that is out of proportion
>to the improvement.
>
>I propose that we hide the compatibility code with ifreq.ifr_addr
>accessors.  In the COMPAT_ cases, the accessors might look like this:
>
>static inline const struct sockaddr *
>ifreq_getaddr(u_long cmd, const struct ifreq *ifr)
>{
>	return &ifr->ifr_addr;
>}
>
>int
>ifreq_setaddr(u_long cmd, struct ifreq *ifr, const struct sockaddr *sa)
>{
>	uint8_t len;
>	const uint8_t osockspace = sizeof(ifr->ifr_addr);
>	const uint8_t sockspace = sizeof(ifr->ifr_ifru.ifru_space);
>
>	switch (cmd) {
>	case OBIOCSETIF:
>	case OSIOCADDMULTI:
>	case OSIOCDELMULTI:
>	case OSIOCSIFADDR:
>	case OSIOCSIFBRDADDR:
>	case OSIOCSIFDSTADDR:
>	case OSIOCSIFFLAGS:
>	case OSIOCSIFNETMASK:
>#ifdef DIAGNOSTIC
>		/* XXX These commands do not copy back to userland.
>		 * XXX
>		 * XXX Set len to 0, or is that too strict?
>		 */
>		printf("%s: kernel discards sockaddr", __func__);
>#endif
>	case OBIOCGETIF:
>	case OOSIOCGIFADDR:
>	case OOSIOCGIFBRDADDR:
>	case OOSIOCGIFCONF:
>	case OOSIOCGIFDSTADDR:
>	case OOSIOCGIFNETMASK:
>	case OSIOCGIFCONF:
>	case OSIOCGIFFLAGS:
>	case OSIOCSIFMEDIA:
>	case OTAPGIFNAME:
>		len = MIN(osockspace, sa->sa_len);
>		break;
>	default:
>		len = MIN(sockspace, sa->sa_len);
>		break;
>	}
>	sockaddr_copy(&ifr->ifr_addr, sa, len);
>	if (len < sa->sa_len)
>		return EFBIG;
>	return 0;
>}
>

This is a good idea. Do you want to do it, or should I?

christos