tech-net archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: bridge sioc[gs]drvspec operations incompatible with COMPAT_NETBSD32
Hi Matt
On 2015-05-30 01:02, Matt Thomas wrote:
The use of SIOC[GS]DRVSPEC to copyin or copyout other structures which
have pointers/size_t/u_long makes them very hard to deal with in
COMPAT_NETBSD32.
The simplest solution is to eliminate the use of the ifbifconf and
ifbaconf structures in userland and have BRDGGIFS and BRDGRTS use the
ifdrv struct members ifd_len and ifb_data directly for their needs.
The netbsd32 compat code already deals with this so this just requires
a small change to if_bridge{.c,var.h}. I also converted ifbareq to
use fixed types in the diff.
Make brconfig to deal with the new method actually makes brconfig
simplier.
There is the problem of missing compat code for the old ifbareq but
I’m not sure if it’s really required.
Comments?
Thanks for working on this!
I took your patch and adjusted it some more:
* Added a check in the kernel if we have a function in the command
table as the ipfilter stuff is optional and I think the table
will now always grow beyond it.
* added an extra parameter to do_cmd in brconfig.c so we can
get the returned length from the ioctl. This allows us to know
if we need a bigger buffer or not.
Seems to be working fine now, at least for setup.
Will be able to plug another interface in once PPPoE works to actually
test it with though.
While in brconfig, I notice that the kernel returns the length needed
if the buffer is too small, but brconfig just does old length *2 and
tries again. Is it worthwile fixing this to grow a buffer of what
the kernel actually wants?
Comments on this welcome!
RoyIndex: sys/net/if_bridge.c
===================================================================
RCS file: /cvsroot/src/sys/net/if_bridge.c,v
retrieving revision 1.98
diff -u -r1.98 if_bridge.c
--- sys/net/if_bridge.c 16 Apr 2015 08:54:15 -0000 1.98
+++ sys/net/if_bridge.c 31 May 2015 09:40:09 -0000
@@ -143,6 +143,10 @@
#include <netinet/ip_carp.h>
#endif
+__CTASSERT(sizeof(struct ifbifconf) == sizeof(struct ifbaconf));
+__CTASSERT(offsetof(struct ifbifconf, ifbic_len) == offsetof(struct ifbaconf, ifbac_len));
+__CTASSERT(offsetof(struct ifbifconf, ifbic_buf) == offsetof(struct ifbaconf, ifbac_buf));
+
/*
* Maximum number of addresses to cache.
*/
@@ -306,6 +310,8 @@
#define BC_F_COPYIN 0x01 /* copy arguments in */
#define BC_F_COPYOUT 0x02 /* copy arguments out */
#define BC_F_SUSER 0x04 /* do super-user check */
+#define BC_F_XLATEIN 0x08 /* xlate arguments in */
+#define BC_F_XLATEOUT 0x10 /* xlate arguments out */
static const struct bridge_control bridge_control_table[] = {
[BRDGADD] = {bridge_ioctl_add, sizeof(struct ifbreq), BC_F_COPYIN|BC_F_SUSER},
@@ -317,8 +323,8 @@
[BRDGSCACHE] = {bridge_ioctl_scache, sizeof(struct ifbrparam), BC_F_COPYIN|BC_F_SUSER},
[BRDGGCACHE] = {bridge_ioctl_gcache, sizeof(struct ifbrparam), BC_F_COPYOUT},
-[BRDGGIFS] = {bridge_ioctl_gifs, sizeof(struct ifbifconf), BC_F_COPYIN|BC_F_COPYOUT},
-[BRDGRTS] = {bridge_ioctl_rts, sizeof(struct ifbaconf), BC_F_COPYIN|BC_F_COPYOUT},
+[OBRDGGIFS] = {bridge_ioctl_gifs, sizeof(struct ifbifconf), BC_F_COPYIN|BC_F_COPYOUT},
+[OBRDGRTS] = {bridge_ioctl_rts, sizeof(struct ifbaconf), BC_F_COPYIN|BC_F_COPYOUT},
[BRDGSADDR] = {bridge_ioctl_saddr, sizeof(struct ifbareq), BC_F_COPYIN|BC_F_SUSER},
@@ -348,7 +354,10 @@
[BRDGGFILT] = {bridge_ioctl_gfilt, sizeof(struct ifbrparam), BC_F_COPYOUT},
[BRDGSFILT] = {bridge_ioctl_sfilt, sizeof(struct ifbrparam), BC_F_COPYIN|BC_F_SUSER},
#endif /* BRIDGE_IPF */
+[BRDGGIFS] = {bridge_ioctl_gifs, sizeof(struct ifbifconf), BC_F_XLATEIN|BC_F_XLATEOUT},
+[BRDGRTS] = {bridge_ioctl_rts, sizeof(struct ifbaconf), BC_F_XLATEIN|BC_F_XLATEOUT},
};
+
static const int bridge_control_table_size = __arraycount(bridge_control_table);
static LIST_HEAD(, bridge_softc) bridge_list;
@@ -627,6 +636,12 @@
}
bc = &bridge_control_table[ifd->ifd_cmd];
+ /* Some commands are optionally included, so check
+ * there is actually a function to call. */
+ if (bc->bc_func == NULL) {
+ error = ENOTSUP;
+ return error;
+ }
/* We only care about BC_F_SUSER at this point. */
if ((bc->bc_flags & BC_F_SUSER) == 0)
@@ -651,20 +666,21 @@
case SIOCSDRVSPEC:
KASSERT(bc != NULL);
if (cmd == SIOCGDRVSPEC &&
- (bc->bc_flags & BC_F_COPYOUT) == 0) {
+ (bc->bc_flags & (BC_F_COPYOUT|BC_F_XLATEOUT)) == 0) {
error = EINVAL;
break;
}
else if (cmd == SIOCSDRVSPEC &&
- (bc->bc_flags & BC_F_COPYOUT) != 0) {
+ (bc->bc_flags & (BC_F_COPYOUT|BC_F_XLATEOUT)) != 0) {
error = EINVAL;
break;
}
/* BC_F_SUSER is checked above, before splnet(). */
- if (ifd->ifd_len != bc->bc_argsize ||
- ifd->ifd_len > sizeof(args)) {
+ if ((bc->bc_flags & (BC_F_XLATEIN|BC_F_XLATEOUT)) == 0
+ && (ifd->ifd_len != bc->bc_argsize
+ || ifd->ifd_len > sizeof(args))) {
error = EINVAL;
break;
}
@@ -674,15 +690,21 @@
error = copyin(ifd->ifd_data, &args, ifd->ifd_len);
if (error)
break;
+ } else if (bc->bc_flags & BC_F_XLATEIN) {
+ args.ifbifconf.ifbic_len = ifd->ifd_len;
+ args.ifbifconf.ifbic_buf = ifd->ifd_data;
}
error = (*bc->bc_func)(sc, &args);
if (error)
break;
- if (bc->bc_flags & BC_F_COPYOUT)
+ if (bc->bc_flags & BC_F_COPYOUT) {
error = copyout(&args, ifd->ifd_data, ifd->ifd_len);
-
+ } else if (bc->bc_flags & BC_F_XLATEOUT) {
+ ifd->ifd_len = args.ifbifconf.ifbic_len;
+ ifd->ifd_data = args.ifbifconf.ifbic_buf;
+ }
break;
case SIOCSIFFLAGS:
Index: sys/net/if_bridgevar.h
===================================================================
RCS file: /cvsroot/src/sys/net/if_bridgevar.h,v
retrieving revision 1.23
diff -u -r1.23 if_bridgevar.h
--- sys/net/if_bridgevar.h 16 Jan 2015 10:36:14 -0000 1.23
+++ sys/net/if_bridgevar.h 31 May 2015 09:40:09 -0000
@@ -90,8 +90,8 @@
#define BRDGSIFFLGS 3 /* set member if flags (ifbreq) */
#define BRDGSCACHE 4 /* set cache size (ifbrparam) */
#define BRDGGCACHE 5 /* get cache size (ifbrparam) */
-#define BRDGGIFS 6 /* get member list (ifbifconf) */
-#define BRDGRTS 7 /* get address list (ifbaconf) */
+#define OBRDGGIFS 6 /* get member list (ifbifconf) */
+#define OBRDGRTS 7 /* get address list (ifbaconf) */
#define BRDGSADDR 8 /* set static address (ifbareq) */
#define BRDGSTO 9 /* set cache timeout (ifbrparam) */
#define BRDGGTO 10 /* get cache timeout (ifbrparam) */
@@ -111,6 +111,9 @@
#define BRDGGFILT 23 /* get filter flags (ifbrparam) */
#define BRDGSFILT 24 /* set filter flags (ifbrparam) */
+#define BRDGGIFS 25 /* get member list */
+#define BRDGRTS 26 /* get address list */
+
/*
* Generic bridge control request.
*/
@@ -163,8 +166,7 @@
*/
struct ifbareq {
char ifba_ifsname[IFNAMSIZ]; /* member if name */
- /*XXX: time_t */
- long ifba_expire; /* address expire time */
+ time_t ifba_expire; /* address expire time */
uint8_t ifba_flags; /* address flags */
uint8_t ifba_dst[ETHER_ADDR_LEN];/* destination address */
};
Index: sbin/brconfig/brconfig.c
===================================================================
RCS file: /cvsroot/src/sbin/brconfig/brconfig.c,v
retrieving revision 1.16
diff -u -r1.16 brconfig.c
--- sbin/brconfig/brconfig.c 28 May 2015 20:14:00 -0000 1.16
+++ sbin/brconfig/brconfig.c 31 May 2015 09:40:09 -0000
@@ -141,7 +141,9 @@
static void show_interfaces(int, const char *, const char *);
static void show_addresses(int, const char *, const char *);
static int get_val(const char *, u_long *);
-static int do_cmd(int, const char *, u_long, void *, size_t, int);
+#define do_cmd(sock, ifname, cmd, buf, buflen, set) \
+ do_cmdl((sock), (ifname), (cmd), (buf), (buflen), NULL, (set))
+static int do_cmdl(int, const char *, u_long, void *, size_t, size_t*, int);
static void do_ifflag(int, const char *, int, int);
static void do_bridgeflag(int, const char *, const char *, int, int);
@@ -418,26 +420,26 @@
"forwarding",
"blocking",
};
- struct ifbifconf bifc;
struct ifbreq *req;
char *inbuf = NULL, *ninbuf;
- uint32_t i, len = 8192;
+ size_t i, len = 8192, olen;
for (;;) {
ninbuf = realloc(inbuf, len);
if (ninbuf == NULL)
err(1, "unable to allocate interface buffer");
- bifc.ifbic_len = len;
- bifc.ifbic_buf = inbuf = ninbuf;
- if (do_cmd(sock, bridge, BRDGGIFS, &bifc, sizeof(bifc), 0) < 0)
+ inbuf = ninbuf;
+ if (do_cmdl(sock, bridge, BRDGGIFS, inbuf, len, &olen, 0) < 0)
err(1, "unable to get interface list");
- if ((bifc.ifbic_len + sizeof(*req)) < len)
+ if (olen <= len)
break;
len *= 2;
}
- for (i = 0; i < bifc.ifbic_len / sizeof(*req); i++) {
- req = bifc.ifbic_req + i;
+ for (i = 0, req = (struct ifbreq *)inbuf;
+ i < olen / sizeof(*req);
+ i++, req++)
+ {
printf("%s%s ", prefix, req->ifbr_ifsname);
printb("flags", req->ifbr_ifsflags, IFBIFBITS);
printf("\n");
@@ -462,31 +464,31 @@
static void
show_addresses(int sock, const char *bridge, const char *prefix)
{
- struct ifbaconf ifbac;
struct ifbareq *ifba;
char *inbuf = NULL, *ninbuf;
- uint32_t i, len = 8192;
+ size_t i, len = 8192, olen;
struct ether_addr ea;
for (;;) {
ninbuf = realloc(inbuf, len);
if (ninbuf == NULL)
err(1, "unable to allocate address buffer");
- ifbac.ifbac_len = len;
- ifbac.ifbac_buf = inbuf = ninbuf;
- if (do_cmd(sock, bridge, BRDGRTS, &ifbac, sizeof(ifbac), 0) < 0)
+ inbuf = ninbuf;
+ if (do_cmdl(sock, bridge, BRDGRTS, inbuf, len, &olen, 0) < 0)
err(1, "unable to get address cache");
- if ((ifbac.ifbac_len + sizeof(*ifba)) < len)
+ if (olen <= len)
break;
len *= 2;
}
- for (i = 0; i < ifbac.ifbac_len / sizeof(*ifba); i++) {
- ifba = ifbac.ifbac_req + i;
+ for (i = 0, ifba = (struct ifbareq *)inbuf;
+ i < olen / sizeof(*ifba);
+ i++, ifba++)
+ {
memcpy(ea.ether_addr_octet, ifba->ifba_dst,
sizeof(ea.ether_addr_octet));
- printf("%s%s %s %ld ", prefix, ether_ntoa(&ea),
- ifba->ifba_ifsname, ifba->ifba_expire);
+ printf("%s%s %s %lld ", prefix, ether_ntoa(&ea),
+ ifba->ifba_ifsname, (long long)ifba->ifba_expire);
printb("flags", ifba->ifba_flags, IFBAFBITS);
printf("\n");
}
@@ -510,10 +512,11 @@
}
static int
-do_cmd(int sock, const char *bridge, u_long op, void *arg, size_t argsize,
- int set)
+do_cmdl(int sock, const char *bridge, u_long op, void *arg, size_t argsize,
+ size_t *outsize, int set)
{
struct ifdrv ifd;
+ int r;
memset(&ifd, 0, sizeof(ifd));
@@ -522,7 +525,11 @@
ifd.ifd_len = argsize;
ifd.ifd_data = arg;
- return (ioctl(sock, set ? SIOCSDRVSPEC : SIOCGDRVSPEC, &ifd));
+ r = ioctl(sock, set ? SIOCSDRVSPEC : SIOCGDRVSPEC, &ifd);
+ if (outsize != NULL && r >= 0)
+ *outsize = ifd.ifd_len;
+
+ return r;
}
static void
Home |
Main Index |
Thread Index |
Old Index