CVS commit: src/sys

[David Young]

Module Name: src
Committed By: christos
Date: Tue May 29 21:32:31 UTC 2007
src/sys/compat/common: Makefile uipc_syscalls_43.c
src/sys/compat/freebsd: freebsd_ioctl.c freebsd_ioctl.h
src/sys/compat/ibcs2: ibcs2_socksys.h
src/sys/compat/linux/common: linux_socket.c
src/sys/compat/sunos: sunos_ioctl.c
src/sys/compat/sunos32: sunos32_ioctl.c
src/sys/compat/svr4: svr4_sockio.c
src/sys/compat/svr4_32: svr4_32_sockio.c
src/sys/compat/sys: socket.h
src/sys/compat/ultrix: ultrix_ioctl.c
src/sys/conf: files
src/sys/net: bpf.c if.c if.h if_etherip.c if_ethersubr.c if_gre.c
if_media.c if_tap.c
src/sys/net80211: ieee80211_ioctl.c
src/sys/sys: ioccom.h sockio.h
src/sys/compat/common: uipc_syscalls_40.c
src/sys/compat/sys: sockio.h
Add a sockaddr_storage member to "struct ifreq" maintaining backwards
compatibility with the older ioctls. This avoids stack smashing and
abuse of "struct sockaddr" when ioctls placed "struct sockaddr_foo's" that
were longer than "struct sockaddr".
XXX: Some of the emulations might be broken; I tried to add code for
them but I did not test them.

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) ?

Dave

[David Young]

Module Name: src
Committed By: christos
Date: Tue May 29 21:32:31 UTC 2007
src/sys/net: bpf.c if.c if.h if_etherip.c if_ethersubr.c if_gre.c
if_media.c if_tap.c

I believe such a change should have been sent to tech-net@ for review
before it was committed.

It looks like you have made unrelated changes to if_ethersubr.c
and if_tap.c, which you have not described in the commit message.

You repeat this code all over:

+#if defined(COMPAT_09) || defined(COMPAT_10) || defined(COMPAT_11) || \
+ defined(COMPAT_12) || defined(COMPAT_13) || defined(COMPAT_14) || \
+ defined(COMPAT_15) || defined(COMPAT_16) || defined(COMPAT_20) || \
+ defined(COMPAT_30) || defined(COMPAT_40)
+#include <compat/sys/sockio.h>
+#endif

Please, move the #ifdef inside of compat/sys/sockio.h, or at least
encapsulate the condition in a #define. People have to read this code.

Finally, gre(4) has grown this wart---no, thank you:

Index: src/sys/net/if_gre.c
diff -u src/sys/net/if_gre.c:1.93 src/sys/net/if_gre.c:1.94
--- src/sys/net/if_gre.c:1.93 Sun May 6 02:47:52 2007
+++ src/sys/net/if_gre.c Tue May 29 21:32:30 2007
@@ -894,13 +902,26 @@
struct sockaddr_in dst, src;
struct proc *p = curproc; /* XXX */
struct lwp *l = curlwp; /* XXX */
- struct ifreq *ifr = (struct ifreq *)data;
+ struct ifreq *ifr;
struct if_laddrreq *lifr = (struct if_laddrreq *)data;
struct gre_softc *sc = ifp->if_softc;
struct sockaddr_in si;
struct sockaddr *sa = NULL;
int error = 0;
-
+ u_long ocmd = cmd;
+#ifdef COMPAT_OIFREQ
+ struct oifreq *oifr;
+ struct ifreq ifrb;
+
+ cmd = cvtcmd(cmd);
+ if (cmd != ocmd) {
+ oifr = data;
+ data = ifr = &ifrb;
+ ifreqo2n(oifr, ifr);
+ } else
+#endif
+ ifr = data;
+
switch (cmd) {
case SIOCSIFFLAGS:
case SIOCSIFMTU:
@@ -1156,6 +1177,10 @@
error = EINVAL;
break;
}
+#ifdef COMPAT_OIFREQ
+ if (cmd != ocmd)
+ ifreqn2o(oifr, ifr);
+#endif
mutex_exit(&sc->sc_mtx);
return error;
}

Dave

[Andrew Doran]

Post by David Young
before it was committed.
It looks like you have made unrelated changes to if_ethersubr.c
and if_tap.c, which you have not described in the commit message.
+#if defined(COMPAT_09) || defined(COMPAT_10) || defined(COMPAT_11) || \
+ defined(COMPAT_12) || defined(COMPAT_13) || defined(COMPAT_14) || \
+ defined(COMPAT_15) || defined(COMPAT_16) || defined(COMPAT_20) || \
+ defined(COMPAT_30) || defined(COMPAT_40)
+#include <compat/sys/sockio.h>
+#endif

Please let's not do stuff like this. Aside from being a pain to manage, we
need to fall back less on macros & compile time options if having a modular
kernel is to be a goal that we can achieve.

Andrew

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Christos Zoulas]

Post by Andrew Doran

Post by David Young
before it was committed.
It looks like you have made unrelated changes to if_ethersubr.c
and if_tap.c, which you have not described in the commit message.
+#if defined(COMPAT_09) || defined(COMPAT_10) || defined(COMPAT_11) || \
+ defined(COMPAT_12) || defined(COMPAT_13) || defined(COMPAT_14) || \
+ defined(COMPAT_15) || defined(COMPAT_16) || defined(COMPAT_20) || \
+ defined(COMPAT_30) || defined(COMPAT_40)
+#include <compat/sys/sockio.h>
+#endif

Please let's not do stuff like this. Aside from being a pain to manage, we
need to fall back less on macros & compile time options if having a modular
kernel is to be a goal that we can achieve.

I will fix this.

christos


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Thor Lancelot Simon]

Post by David Young
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...

[der Mouse]

sockaddr_storage is, by definition, large enough to contain any
protocol-specific sockaddr. That's what it's for.

Then it needs to be enlarged substantially; the only limit on AF_LOCAL
sockaddr size I can find is the one implied by the various *_len fields
(including sun_len) being unsigned char. (I consider this limit a bug,
especially as it's not documented as far as I can see. unix(4) implies
the limit is 104, without giving a manifest constant for the limit,
without explaining why it's 104 and not, say, 126, and without
explaining that the actual limit is entirely different.)

Of course, AF_LOCAL is broken in various other ways too, so there's a
certain amount of "what's one more"....

/~\ The ASCII der Mouse
\ / Ribbon Campaign
X Against HTML ***@rodents.montreal.qc.ca
/ \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[David Young]

Post by Thor Lancelot Simon

Post by David Young
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

[Christos Zoulas]

Post by David Young

Post by Thor Lancelot Simon

Post by David Young
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
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) {
#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
len = MIN(osockspace, sa->sa_len);
break;
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


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[David Young]

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

I will do it.

Dave

[Thor Lancelot Simon]

Post by der Mouse

sockaddr_storage is, by definition, large enough to contain any
protocol-specific sockaddr. That's what it's for.

Then it needs to be enlarged substantially; the only limit on AF_LOCAL
sockaddr size I can find is the one implied by the various *_len fields
(including sun_len) being unsigned char. (I consider this limit a bug,
especially as it's not documented as far as I can see. unix(4) implies
the limit is 104, without giving a manifest constant for the limit,
without explaining why it's 104 and not, say, 126, and without
explaining that the actual limit is entirely different.)

Without taking up the other issue (where I mostly agree with you): I don't
think _this_ use of sockaddr_storage in a structure is a problem with
regard to AF_LOCAL, because you can't do an interface ioctls on an AF_LOCAL
socket: it has no interface.

If there are other sockaddrs hiding in structures sent up to or down from
the kernel, I agree, the size needs to get larger to handle AF_LOCAL
addresses, the length on the limit of which should, in fact, be documented,
regardless.

I think a sockaddr_storage is 256 bytes on FreeBSD. Maybe it should be
here, too, to address this issue. But *that* would, I think, present a
whole new set of backwards compatibility issues.

Thor

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Christos Zoulas]

Post by Thor Lancelot Simon

Post by der Mouse

sockaddr_storage is, by definition, large enough to contain any
protocol-specific sockaddr. That's what it's for.

Then it needs to be enlarged substantially; the only limit on AF_LOCAL
sockaddr size I can find is the one implied by the various *_len fields
(including sun_len) being unsigned char. (I consider this limit a bug,
especially as it's not documented as far as I can see. unix(4) implies
the limit is 104, without giving a manifest constant for the limit,
without explaining why it's 104 and not, say, 126, and without
explaining that the actual limit is entirely different.)

Without taking up the other issue (where I mostly agree with you): I don't
think _this_ use of sockaddr_storage in a structure is a problem with
regard to AF_LOCAL, because you can't do an interface ioctls on an AF_LOCAL
socket: it has no interface.
If there are other sockaddrs hiding in structures sent up to or down from
the kernel, I agree, the size needs to get larger to handle AF_LOCAL
addresses, the length on the limit of which should, in fact, be documented,
regardless.
I think a sockaddr_storage is 256 bytes on FreeBSD. Maybe it should be
here, too, to address this issue. But *that* would, I think, present a
whole new set of backwards compatibility issues.

It is 128 on FreeBSD too. I think that we can make it 256 *right now*
without causing any grief and riding on the ifreq changes. But yes, if
we wait longer we will need to deal with another ifreq versioning. Also
we could just leave sockaddr_storage alone and add another member in
ifru that is let's say 256 or 512. Should we?

christos


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[der Mouse]

[...]. Suppose the kernel copies to ifreq a sockaddr whose sa_len >
sizeof(struct sockaddr_storage) ?

...which is nto all that implausible; AF_LOCAL sockets can have more or
less unlimited sun_len - well, they should be able to; our
implementation limits them to 255 (because it's stored in char), which
I consider a bug. But 255 > sizeof(struct sockaddr_storage)....

/~\ The ASCII der Mouse
\ / Ribbon Campaign
X Against HTML ***@rodents.montreal.qc.ca
/ \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Quentin Garnier]

Post by David Young

Module Name: src
Committed By: christos
Date: Tue May 29 21:32:31 UTC 2007
src/sys/compat/common: Makefile uipc_syscalls_43.c
src/sys/compat/freebsd: freebsd_ioctl.c freebsd_ioctl.h
src/sys/compat/ibcs2: ibcs2_socksys.h
src/sys/compat/linux/common: linux_socket.c
src/sys/compat/sunos: sunos_ioctl.c
src/sys/compat/sunos32: sunos32_ioctl.c
src/sys/compat/svr4: svr4_sockio.c
src/sys/compat/svr4_32: svr4_32_sockio.c
src/sys/compat/sys: socket.h
src/sys/compat/ultrix: ultrix_ioctl.c
src/sys/conf: files
src/sys/net: bpf.c if.c if.h if_etherip.c if_ethersubr.c if_gre.c
if_media.c if_tap.c
src/sys/net80211: ieee80211_ioctl.c
src/sys/sys: ioccom.h sockio.h
src/sys/compat/common: uipc_syscalls_40.c
src/sys/compat/sys: sockio.h
Add a sockaddr_storage member to "struct ifreq" maintaining backwards
compatibility with the older ioctls. This avoids stack smashing and
abuse of "struct sockaddr" when ioctls placed "struct sockaddr_foo's" that
were longer than "struct sockaddr".
XXX: Some of the emulations might be broken; I tried to add code for
them but I did not test them.

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) ?

Stack smashing is only a side effect of the issue of having a sockaddr
in ifreq when it's meant to hold a larger, AF-dependent, structure.

As for the length, sockaddr_storage now essentially works as the maximum
size a sockaddr object is allowed to be.