In article <20070530005638.GE25295 che.ojctech.com>,
David Young <dyoungpobox.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
|
