Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 28 Apr 2012 23:18:00 +0900 (JST)
From:      Hiroki Sato <hrs@FreeBSD.org>
To:        melifaro@FreeBSD.org, kudzu@tenebras.com
Cc:        freebsd-ipfw@FreeBSD.org
Subject:   Re: CFR: ipfw0 pseudo-interface clonable
Message-ID:  <20120428.231800.306465812317617923.hrs@allbsd.org>
In-Reply-To: <20120425.020518.406495893112283552.hrs@allbsd.org> <CAHu1Y70kbxu6dquUKvrLwQwjLBuCiv2wu7QitoUHro17=rjbGA@mail.gmail.com>
References:  <20120425.002600.1631867625819249738.hrs@allbsd.org> <4F96D11B.2060007@FreeBSD.org> <20120425.020518.406495893112283552.hrs@allbsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
----Security_Multipart0(Sat_Apr_28_23_18_00_2012_214)--
Content-Type: Multipart/Mixed;
	boundary="--Next_Part(Sat_Apr_28_23_18_00_2012_764)--"
Content-Transfer-Encoding: 7bit

----Next_Part(Sat_Apr_28_23_18_00_2012_764)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

Hiroki Sato <hrs@freebsd.org> wrote
  in <20120425.020518.406495893112283552.hrs@allbsd.org>:

hr> "Alexander V. Chernikov" <melifaro@FreeBSD.org> wrote
hr>   in <4F96D11B.2060007@FreeBSD.org>:
hr>
hr> me> On 24.04.2012 19:26, Hiroki Sato wrote:
hr> me> > Hi,
hr> me> >
hr> me> >   I created the attached patch to make the current ipfw0
hr> me> >   pseudo-interface clonable.  The functionality of ipfw0 logging
hr> me> >   interface is not changed by this patch, but the ipfw0
hr> me> >   pseudo-interface is not created by default and can be created with
hr> me> >   the following command:

hr> me> ipfw_log() log_if usage is not protected, so it is possible to trigger
hr> me> use-after-free.
hr>
hr>  Ah, right.  I will revise lock handling and resubmit the patch.

Michael Sierchio <kudzu@tenebras.com> wrote
  in <CAHu1Y70kbxu6dquUKvrLwQwjLBuCiv2wu7QitoUHro17=rjbGA@mail.gmail.com>:

ku> A man page for the ipfw pseudo-interface would be welcome.

 A revised patch is attached.  The lock around log_if should be fixed
 and ipfw(8) manual page is updated.  Also, an rc.conf(5) variable
 $firewall_logif is added to create ipfw0 interface at boot time (NO
 by default).

 Any comments are welcome.  Thank you.

-- Hiroki

----Next_Part(Sat_Apr_28_23_18_00_2012_764)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline; filename="ipfw_clone_interface.20120429-1.diff"

Index: sys/netinet/ipfw/ip_fw_log.c
===================================================================
--- sys/netinet/ipfw/ip_fw_log.c	(revision 234428)
+++ sys/netinet/ipfw/ip_fw_log.c	(working copy)
@@ -44,8 +44,11 @@
 #include <sys/socket.h>
 #include <sys/sysctl.h>
 #include <sys/syslog.h>
+#include <sys/lock.h>
+#include <sys/rwlock.h>
 #include <net/ethernet.h> /* for ETHERTYPE_IP */
 #include <net/if.h>
+#include <net/if_clone.h>
 #include <net/vnet.h>
 #include <net/if_types.h>	/* for IFT_ETHER */
 #include <net/bpf.h>		/* for BPF */
@@ -90,7 +93,16 @@
 }
 #else /* !WITHOUT_BPF */
 static struct ifnet *log_if;	/* hook to attach to bpf */
+static struct rwlock log_if_lock;
+#define	LOGIF_LOCK_INIT(x)	rw_init(&log_if_lock, "ipfw log_if lock")
+#define	LOGIF_LOCK_DESTROY(x)	rw_destroy(&log_if_lock)
+#define	LOGIF_RLOCK(x)		rw_rlock(&log_if_lock)
+#define	LOGIF_RUNLOCK(x)	rw_runlock(&log_if_lock)
+#define	LOGIF_WLOCK(x)		rw_wlock(&log_if_lock)
+#define	LOGIF_WUNLOCK(x)	rw_wunlock(&log_if_lock)

+#define	IPFWNAME	"ipfw"
+
 /* we use this dummy function for all ifnet callbacks */
 static int
 log_dummy(struct ifnet *ifp, u_long cmd, caddr_t addr)
@@ -116,37 +128,104 @@
 static const u_char ipfwbroadcastaddr[6] =
 	{ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };

+static int
+ipfw_log_clone_match(struct if_clone *ifc, const char *name)
+{
+
+	return (strncmp(name, IPFWNAME, sizeof(IPFWNAME) - 1) == 0);
+}
+
+static int
+ipfw_log_clone_create(struct if_clone *ifc, char *name, size_t len, caddr_t params)
+{
+	int error;
+	int unit;
+	struct ifnet *ifp;
+
+	error = ifc_name2unit(name, &unit);
+	if (error)
+		return (error);
+
+	error = ifc_alloc_unit(ifc, &unit);
+	if (error)
+		return (error);
+
+	ifp = if_alloc(IFT_ETHER);
+	if (ifp == NULL) {
+		ifc_free_unit(ifc, unit);
+		return (ENOSPC);
+	}
+	ifp->if_dname = IPFWNAME;
+	ifp->if_dunit = unit;
+	snprintf(ifp->if_xname, IFNAMSIZ, "%s%d", IPFWNAME, unit);
+	strlcpy(name, ifp->if_xname, len);
+	ifp->if_mtu = 65536;
+	ifp->if_flags = IFF_UP | IFF_SIMPLEX | IFF_MULTICAST;
+	ifp->if_init = (void *)log_dummy;
+	ifp->if_ioctl = log_dummy;
+	ifp->if_start = ipfw_log_start;
+	ifp->if_output = ipfw_log_output;
+	ifp->if_addrlen = 6;
+	ifp->if_hdrlen = 14;
+	ifp->if_broadcastaddr = ipfwbroadcastaddr;
+	ifp->if_baudrate = IF_Mbps(10);
+
+	LOGIF_WLOCK();
+	if (log_if == NULL)
+		log_if = ifp;
+	else {
+		LOGIF_WUNLOCK();
+		if_free(ifp);
+		ifc_free_unit(ifc, unit);
+		return (EEXIST);
+	}
+	LOGIF_WUNLOCK();
+	if_attach(ifp);
+	bpfattach(ifp, DLT_EN10MB, 14);
+
+	return (0);
+}
+
+static int
+ipfw_log_clone_destroy(struct if_clone *ifc, struct ifnet *ifp)
+{
+	int unit;
+
+	if (ifp == NULL)
+		return (0);
+
+	LOGIF_WLOCK();
+	if (log_if != NULL && ifp == log_if)
+		log_if = NULL;
+	else {
+		LOGIF_WUNLOCK();
+		return (EINVAL);
+	}
+	LOGIF_WUNLOCK();
+
+	unit = ifp->if_dunit;
+	bpfdetach(ifp);
+	if_detach(ifp);
+	if_free(ifp);
+	ifc_free_unit(ifc, unit);
+
+	return (0);
+}
+
+static struct if_clone ipfw_log_cloner = IFC_CLONE_INITIALIZER(
+    IPFWNAME, NULL, IF_MAXUNIT,
+    NULL, ipfw_log_clone_match, ipfw_log_clone_create, ipfw_log_clone_destroy);
+
 void
 ipfw_log_bpf(int onoff)
 {
-	struct ifnet *ifp;

 	if (onoff) {
-		if (log_if)
-			return;
-		ifp = if_alloc(IFT_ETHER);
-		if (ifp == NULL)
-			return;
-		if_initname(ifp, "ipfw", 0);
-		ifp->if_mtu = 65536;
-		ifp->if_flags = IFF_UP | IFF_SIMPLEX | IFF_MULTICAST;
-		ifp->if_init = (void *)log_dummy;
-		ifp->if_ioctl = log_dummy;
-		ifp->if_start = ipfw_log_start;
-		ifp->if_output = ipfw_log_output;
-		ifp->if_addrlen = 6;
-		ifp->if_hdrlen = 14;
-		if_attach(ifp);
-		ifp->if_broadcastaddr = ipfwbroadcastaddr;
-		ifp->if_baudrate = IF_Mbps(10);
-		bpfattach(ifp, DLT_EN10MB, 14);
-		log_if = ifp;
+		LOGIF_LOCK_INIT();
+		if_clone_attach(&ipfw_log_cloner);
 	} else {
-		if (log_if) {
-			ether_ifdetach(log_if);
-			if_free(log_if);
-		}
-		log_if = NULL;
+		if_clone_detach(&ipfw_log_cloner);
+		LOGIF_LOCK_DESTROY();
 	}
 }
 #endif /* !WITHOUT_BPF */
@@ -166,9 +245,11 @@

 	if (V_fw_verbose == 0) {
 #ifndef WITHOUT_BPF
-
-		if (log_if == NULL || log_if->if_bpf == NULL)
+		LOGIF_RLOCK();
+		if (log_if == NULL || log_if->if_bpf == NULL) {
+			LOGIF_RUNLOCK();
 			return;
+		}

 		if (args->eh) /* layer2, use orig hdr */
 			BPF_MTAP2(log_if, args->eh, ETHER_HDR_LEN, m);
@@ -177,6 +258,7 @@
 			 * more info in the header.
 			 */
 			BPF_MTAP2(log_if, "DDDDDDSSSSSS\x08\x00", ETHER_HDR_LEN, m);
+		LOGIF_RUNLOCK();
 #endif /* !WITHOUT_BPF */
 		return;
 	}
Index: sbin/ipfw/ipfw.8
===================================================================
--- sbin/ipfw/ipfw.8	(revision 234428)
+++ sbin/ipfw/ipfw.8	(working copy)
@@ -1,7 +1,7 @@
 .\"
 .\" $FreeBSD$
 .\"
-.Dd March 9, 2012
+.Dd April 28, 2012
 .Dt IPFW 8
 .Os
 .Sh NAME
@@ -560,7 +560,22 @@
 .Xr bpf 4
 attached to the
 .Li ipfw0
-pseudo interface. There is no overhead if no
+pseudo interface.
+This pseudo interface can be created after a boot
+manually by using the following command:
+.Bd -literal -offset indent
+# ifconfig ipfw0 create
+.Ed
+.Pp
+Or, automatically at boot time by adding the following
+line to the
+.Xr rc.conf 5
+file:
+.Bd -literal -offset indent
+firewall_logif="YES"
+.Ed
+.Pp
+There is no overhead if no
 .Xr bpf 4
 is attached to the pseudo interface.
 .Pp
Index: etc/rc.d/ipfw
===================================================================
--- etc/rc.d/ipfw	(revision 234412)
+++ etc/rc.d/ipfw	(working copy)
@@ -57,6 +57,10 @@
 		echo 'Firewall logging enabled.'
 		sysctl net.inet.ip.fw.verbose=1 >/dev/null
 	fi
+	if checkyesno firewall_logif; then
+		echo 'Firewall logging pseudo-interface (ipfw0) created.'
+		ifconfig ipfw0 create
+	fi
 }

 ipfw_poststart()
Index: etc/defaults/rc.conf
===================================================================
--- etc/defaults/rc.conf	(revision 234428)
+++ etc/defaults/rc.conf	(working copy)
@@ -123,6 +123,7 @@
 firewall_type="UNKNOWN"		# Firewall type (see /etc/rc.firewall)
 firewall_quiet="NO"		# Set to YES to suppress rule display
 firewall_logging="NO"		# Set to YES to enable events logging
+firewall_logif="NO"		# Set to YES to create logging-pseudo interface
 firewall_flags=""		# Flags passed to ipfw when type is a file
 firewall_coscripts=""		# List of executables/scripts to run after
 				# firewall starts/stops

----Next_Part(Sat_Apr_28_23_18_00_2012_764)----

----Security_Multipart0(Sat_Apr_28_23_18_00_2012_214)--
Content-Type: application/pgp-signature
Content-Transfer-Encoding: 7bit

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (FreeBSD)

iEYEABECAAYFAk+b/BgACgkQTyzT2CeTzy2fdwCghazmRs6QeMA0631ZY3CGyTNC
oDwAoIlL9GEFhmLy7Bw7epRAU9swB+EY
=MiFt
-----END PGP SIGNATURE-----

----Security_Multipart0(Sat_Apr_28_23_18_00_2012_214)----



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120428.231800.306465812317617923.hrs>