bridge sioc[gs]drvspec operations incompatible with COMPAT_NETBSD32

[Matt Thomas <matt%3am-software.com@localhost>]

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.

? sys/net/z
Index: sys/net/if_bridge.c
===================================================================
RCS file: /cvsroot/src/sys/net/if_bridge.c,v
retrieving revision 1.98
diff -u -p -r1.98 if_bridge.c
--- sys/net/if_bridge.c	16 Apr 2015 08:54:15 -0000	1.98
+++ sys/net/if_bridge.c	29 May 2015 23:52:48 -0000
@@ -143,6 +143,10 @@ __KERNEL_RCSID(0, "$NetBSD: if_bridge.c,
 #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 @@ struct bridge_control {
 #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 @@ static const struct bridge_control bridg
 [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 @@ static const struct bridge_control bridg
 [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;
@@ -651,20 +660,21 @@ bridge_ioctl(struct ifnet *ifp, u_long c
 	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 +684,21 @@ bridge_ioctl(struct ifnet *ifp, u_long c
 			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 -p -r1.23 if_bridgevar.h
--- sys/net/if_bridgevar.h	16 Jan 2015 10:36:14 -0000	1.23
+++ sys/net/if_bridgevar.h	29 May 2015 23:52:48 -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 ifbifconf {
  */
 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 -p -r1.16 brconfig.c
--- sbin/brconfig/brconfig.c	28 May 2015 20:14:00 -0000	1.16
+++ sbin/brconfig/brconfig.c	29 May 2015 23:58:35 -0000
@@ -418,7 +418,6 @@ show_interfaces(int sock, const char *br
 		"forwarding",
 		"blocking",
 	};
-	struct ifbifconf bifc;
 	struct ifbreq *req;
 	char *inbuf = NULL, *ninbuf;
 	uint32_t i, len = 8192;
@@ -427,9 +426,8 @@ show_interfaces(int sock, const char *br
 		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_cmd(sock, bridge, BRDGGIFS, inbuf, len, 0) < 0)
 			err(1, "unable to get interface list");
 		if ((bifc.ifbic_len + sizeof(*req)) < len)
 			break;
@@ -462,7 +460,6 @@ show_interfaces(int sock, const char *br
 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;
@@ -472,9 +469,8 @@ show_addresses(int sock, const char *bri
 		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_cmd(sock, bridge, BRDGRTS, inbuf, len, 0) < 0)
 			err(1, "unable to get address cache");
 		if ((ifbac.ifbac_len + sizeof(*ifba)) < len)
 			break;


There is the problem of missing compat code for the old ifbareq but I’m not sure if it’s really required.

Comments?