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;
}

Dave

-- 
David Young             OJC Technologies
dyoung@ojctech.com      Urbana, IL * (217) 278-3933 ext 24