Date: Sun, 10 Jun 2012 18:20:25 +0400 From: "Alexander V. Chernikov" <melifaro@FreeBSD.org> To: Hiroki Sato <hrs@FreeBSD.org> Cc: freebsd-ipfw@FreeBSD.org Subject: Re: CFR: ipfw0 pseudo-interface clonable Message-ID: <4FD4AD29.3040204@FreeBSD.org> In-Reply-To: <20120427.084414.1142593201575277510.hrs@allbsd.org> References: <4F96D11B.2060007@FreeBSD.org> <20120425.020518.406495893112283552.hrs@allbsd.org> <4F96E71B.9020405@FreeBSD.org> <20120427.084414.1142593201575277510.hrs@allbsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format.
--------------070804010107020305000604
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
On 27.04.2012 03:44, Hiroki Sato wrote:
> "Alexander V. Chernikov"<melifaro@FreeBSD.org> wrote
> in<4F96E71B.9020405@FreeBSD.org>:
>
> me> On 24.04.2012 21:05, Hiroki Sato wrote:
> me> > "Alexander V. Chernikov"<melifaro@FreeBSD.org> wrote
> me> > in<4F96D11B.2060007@FreeBSD.org>:
> me> >
> me> > me> On 24.04.2012 19:26, Hiroki Sato wrote:
> me> > me> > Any objection to commit this patch? The primary motivation for
> me> > this
> me> > me> > change is that presence of the interface by default increases
> me> > size of
> me> > me> > the interface list, which is returned by NET_RT_IFLIST sysctl
> me> > even
> me> > me> > when the sysadmin does not need it. Also this pseudo-interface
> me> > can
> me> > me> > confuse the sysadmin and/or network-related userland utilities
> me> > like
> me> > me> > SNMP agent. With this patch, one can use ifconfig(8) to
> me> > me> > create/destroy the pseudo-interface as necessary.
> me> > me>
> me> > me> ipfw_log() log_if usage is not protected, so it is possible to
> me> > trigger
> me> > me> use-after-free.
> me> >
> me> > Ah, right. I will revise lock handling and resubmit the patch.
> me> >
> me> > me> Maybe it is better to have some interface flag which makes
> me> > me> NET_RT_IFLIST skip given interface ?
> me> >
> me> > I do not think so. NET_RT_IFLIST should be able to list all of the
> me> > interfaces because it is the purpose.
> me> Okay, another try (afair already discussed somewhere):
> me> Do we really need all BPF providers to have ifnets?
> me> It seems that removing all bp_bif depends from BPF code is not so hard
> me> task.
>
> Hmm, I cannot imagine how to decouple ifnet from the bpf code because
> bpf heavily depends on it in its API (you probably know better than
> me). Do you have any specific idea?
Proof-of-concept patch attached.
Unfortunately, there are problems with this approach, too.
pcap_findalldevs() uses external to BPF method (possibly NET_RT_IFLIST),
so programs relying on that function for showing some kind of combo-box
(like wireshark) with all possible variant won't allow user to specify
such interface.
Additionally, tcpdump assumes that passed interface name is real and
warns us that SIOCGIFADDR returns error.
>
> -- Hiroki
--------------070804010107020305000604
Content-Type: text/plain;
name="bpf_fake.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="bpf_fake.diff"
diff --git a/sys/net/bpf.c b/sys/net/bpf.c
index 6bff58e..d8ecc02 100644
--- a/sys/net/bpf.c
+++ b/sys/net/bpf.c
@@ -654,7 +654,7 @@ bpf_attachd(struct bpf_d *d, struct bpf_if *bp)
CTR3(KTR_NET, "%s: bpf_attach called by pid %d, adding to %s list",
__func__, d->bd_pid, d->bd_writer ? "writer" : "active");
- if (op_w == 0)
+ if ((op_w == 0) && (bp->bif_ifp != NULL))
EVENTHANDLER_INVOKE(bpf_track, bp->bif_ifp, bp->bif_dlt, 1);
}
@@ -696,7 +696,8 @@ bpf_upgraded(struct bpf_d *d)
CTR2(KTR_NET, "%s: upgrade required by pid %d", __func__, d->bd_pid);
- EVENTHANDLER_INVOKE(bpf_track, bp->bif_ifp, bp->bif_dlt, 1);
+ if (bp->bif_ifp != NULL)
+ EVENTHANDLER_INVOKE(bpf_track, bp->bif_ifp, bp->bif_dlt, 1);
}
/*
@@ -744,14 +745,14 @@ bpf_detachd_locked(struct bpf_d *d)
bpf_bpfd_cnt--;
/* Call event handler iff d is attached */
- if (error == 0)
+ if ((error == 0) && (ifp != NULL))
EVENTHANDLER_INVOKE(bpf_track, ifp, bp->bif_dlt, 0);
/*
* Check if this descriptor had requested promiscuous mode.
* If so, turn it off.
*/
- if (d->bd_promisc) {
+ if (d->bd_promisc && ifp != NULL) {
d->bd_promisc = 0;
CURVNET_SET(ifp->if_vnet);
error = ifpromisc(ifp, 0);
@@ -1034,7 +1035,10 @@ bpfwrite(struct cdev *dev, struct uio *uio, int ioflag)
return (ENXIO);
}
- ifp = d->bd_bif->bif_ifp;
+ if ((ifp = d->bd_bif->bif_ifp) == NULL) {
+ d->bd_wdcount++;
+ return (ENXIO);
+ }
if ((ifp->if_flags & IFF_UP) == 0) {
d->bd_wdcount++;
@@ -1266,8 +1270,10 @@ bpfioctl(struct cdev *dev, u_long cmd, caddr_t addr, int flags,
if (d->bd_bif == NULL)
error = EINVAL;
else {
- ifp = d->bd_bif->bif_ifp;
- error = (*ifp->if_ioctl)(ifp, cmd, addr);
+ if ((ifp = d->bd_bif->bif_ifp) == NULL)
+ error = EINVAL;
+ else
+ error = (*ifp->if_ioctl)(ifp, cmd, addr);
}
break;
}
@@ -1322,6 +1328,13 @@ bpfioctl(struct cdev *dev, u_long cmd, caddr_t addr, int flags,
error = EINVAL;
break;
}
+
+ if (d->bd_bif->bif_ifp == NULL) {
+ /* Silently ignore fake interfaces */
+ error = 0;
+ break;
+ }
+
if (d->bd_promisc == 0) {
error = ifpromisc(d->bd_bif->bif_ifp, 1);
if (error == 0)
@@ -1398,8 +1411,13 @@ bpfioctl(struct cdev *dev, u_long cmd, caddr_t addr, int flags,
struct ifnet *const ifp = d->bd_bif->bif_ifp;
struct ifreq *const ifr = (struct ifreq *)addr;
- strlcpy(ifr->ifr_name, ifp->if_xname,
- sizeof(ifr->ifr_name));
+ if (ifp == NULL) {
+ /* Fake interface */
+ strlcpy(ifr->ifr_name, d->bd_bif->ifname,
+ sizeof(ifr->ifr_name));
+ } else
+ strlcpy(ifr->ifr_name, ifp->if_xname,
+ sizeof(ifr->ifr_name));
}
BPF_UNLOCK();
break;
@@ -1844,10 +1862,19 @@ bpf_setif(struct bpf_d *d, struct ifreq *ifr)
BPF_LOCK_ASSERT();
theywant = ifunit(ifr->ifr_name);
- if (theywant == NULL || theywant->if_bpf == NULL)
- return (ENXIO);
+ if (theywant == NULL || theywant->if_bpf == NULL) {
+ /* Check for fake interface existance */
+ LIST_FOREACH(bp, &bpf_iflist, bif_next) {
+ if (bp->bif_ifp != NULL)
+ continue;
+ if (!strcmp(bp->ifname, ifr->ifr_name))
+ break;
+ }
- bp = theywant->if_bpf;
+ if (bp == NULL)
+ return (ENXIO);
+ } else
+ bp = theywant->if_bpf;
/* Check if interface is not being detached from BPF */
BPFIF_RLOCK(bp);
@@ -2072,7 +2099,8 @@ bpf_tap(struct bpf_if *bp, u_char *pkt, u_int pktlen)
if (gottime < bpf_ts_quality(d->bd_tstamp))
gottime = bpf_gettime(&bt, d->bd_tstamp, NULL);
#ifdef MAC
- if (mac_bpfdesc_check_receive(d, bp->bif_ifp) == 0)
+ if (bp->bif_ifp == NULL ||
+ (mac_bpfdesc_check_receive(d, bp->bif_ifp) == 0))
#endif
catchpacket(d, pkt, pktlen, slen,
bpf_append_bytes, &bt);
@@ -2082,6 +2110,7 @@ bpf_tap(struct bpf_if *bp, u_char *pkt, u_int pktlen)
BPFIF_RUNLOCK(bp);
}
+/* Note i CAN be NULL */
#define BPF_CHECK_DIRECTION(d, r, i) \
(((d)->bd_direction == BPF_D_IN && (r) != (i)) || \
((d)->bd_direction == BPF_D_OUT && (r) == (i)))
@@ -2131,7 +2160,8 @@ bpf_mtap(struct bpf_if *bp, struct mbuf *m)
if (gottime < bpf_ts_quality(d->bd_tstamp))
gottime = bpf_gettime(&bt, d->bd_tstamp, m);
#ifdef MAC
- if (mac_bpfdesc_check_receive(d, bp->bif_ifp) == 0)
+ if ((bp->bif_ifp == NULL) ||
+ (mac_bpfdesc_check_receive(d, bp->bif_ifp) == 0))
#endif
catchpacket(d, (u_char *)m, pktlen, slen,
bpf_append_mbuf, &bt);
@@ -2187,7 +2217,8 @@ bpf_mtap2(struct bpf_if *bp, void *data, u_int dlen, struct mbuf *m)
if (gottime < bpf_ts_quality(d->bd_tstamp))
gottime = bpf_gettime(&bt, d->bd_tstamp, m);
#ifdef MAC
- if (mac_bpfdesc_check_receive(d, bp->bif_ifp) == 0)
+ if ((bp->bif_ifp == NULL) ||
+ (mac_bpfdesc_check_receive(d, bp->bif_ifp) == 0))
#endif
catchpacket(d, (u_char *)&mb, pktlen, slen,
bpf_append_mbuf, &bt);
@@ -2481,6 +2512,44 @@ bpfattach2(struct ifnet *ifp, u_int dlt, u_int hdrlen, struct bpf_if **driverp)
}
/*
+ * Attach fake interface to bpf. ifname is interface name to be attached,
+ * dlt is the link layer type, and hdrlen is the fixed size of the link header
+ * (variable length headers are not yet supporrted).
+ */
+void
+bpfattach3(char *ifname, u_int dlt, u_int hdrlen, struct bpf_if **driverp)
+{
+ struct bpf_if *bp;
+ int len;
+
+ len = strlen(ifname) + 1;
+
+ /* Assume bpf_if to be properly aligned */
+ bp = malloc(sizeof(*bp) + len, M_BPF, M_NOWAIT | M_ZERO);
+ if (bp == NULL)
+ panic("bpfattach");
+
+ LIST_INIT(&bp->bif_dlist);
+ LIST_INIT(&bp->bif_wlist);
+ bp->ifname = (char *)(bp + 1);
+ strlcpy(bp->ifname, ifname, len);
+ bp->bif_dlt = dlt;
+ rw_init(&bp->bif_lock, "bpf interface lock");
+ KASSERT(*driverp == NULL, ("bpfattach3: driverp already initialized"));
+ *driverp = bp;
+
+ BPF_LOCK();
+ LIST_INSERT_HEAD(&bpf_iflist, bp, bif_next);
+ BPF_UNLOCK();
+
+ bp->bif_hdrlen = hdrlen;
+
+ if (bootverbose)
+ printf("%s: bpf attached\n", bp->ifname);
+}
+
+
+/*
* Detach bpf from an interface. This involves detaching each descriptor
* associated with the interface. Notify each descriptor as it's detached
* so that any sleepers wake up and get ENXIO.
@@ -2543,6 +2612,67 @@ bpfdetach(struct ifnet *ifp)
}
/*
+ * Detach bpf from the fake interface. This involves detaching each descriptor
+ * associated with the interface. Notify each descriptor as it's detached
+ * so that any sleepers wake up and get ENXIO.
+ */
+void
+bpfdetach3(char *ifname)
+{
+ struct bpf_if *bp;
+ struct bpf_d *d;
+#ifdef INVARIANTS
+ int ndetached;
+
+ ndetached = 0;
+#endif
+
+ BPF_LOCK();
+ /* Find all bpf_if struct's which reference ifp and detach them. */
+ do {
+ LIST_FOREACH(bp, &bpf_iflist, bif_next) {
+ if (bp->bif_ifp != NULL)
+ continue;
+ if (!strcmp(bp->ifname, ifname))
+ break;
+ }
+ if (bp != NULL)
+ LIST_REMOVE(bp, bif_next);
+
+ if (bp != NULL) {
+#ifdef INVARIANTS
+ ndetached++;
+#endif
+ while ((d = LIST_FIRST(&bp->bif_dlist)) != NULL) {
+ bpf_detachd_locked(d);
+ BPFD_LOCK(d);
+ bpf_wakeup(d);
+ BPFD_UNLOCK(d);
+ }
+ /* Free writer-only descriptors */
+ while ((d = LIST_FIRST(&bp->bif_wlist)) != NULL) {
+ bpf_detachd_locked(d);
+ BPFD_LOCK(d);
+ bpf_wakeup(d);
+ BPFD_UNLOCK(d);
+ }
+
+ /*
+ * Since this interface is fake we can free our
+ * structure immediately.
+ */
+ rw_destroy(&bp->bif_lock);
+ free(bp, M_BPF);
+ }
+ } while (bp != NULL);
+ BPF_UNLOCK();
+
+#ifdef INVARIANTS
+ if (ndetached == 0)
+ printf("bpfdetach: %s was not attached\n", ifname);
+#endif
+}
+/*
* Interface departure handler.
* Note departure event does not guarantee interface is going down.
*/
@@ -2591,6 +2721,9 @@ bpf_getdltlist(struct bpf_d *d, struct bpf_dltlist *bfl)
LIST_FOREACH(bp, &bpf_iflist, bif_next) {
if (bp->bif_ifp != ifp)
continue;
+ /* Compare fake interfaces by name */
+ if (ifp == NULL && strcmp(d->bd_bif->ifname, bp->ifname))
+ continue;
if (bfl->bfl_list != NULL) {
if (n >= bfl->bfl_len)
return (ENOMEM);
@@ -2620,7 +2753,13 @@ bpf_setdlt(struct bpf_d *d, u_int dlt)
ifp = d->bd_bif->bif_ifp;
LIST_FOREACH(bp, &bpf_iflist, bif_next) {
- if (bp->bif_ifp == ifp && bp->bif_dlt == dlt)
+ if (bp->bif_ifp != ifp)
+ continue;
+
+ if (ifp == NULL && strcmp(d->bd_bif->ifname, bp->ifname))
+ continue;
+
+ if (bp->bif_dlt == dlt)
break;
}
@@ -2715,8 +2854,10 @@ bpfstats_fill_xbpf(struct xbpf_d *d, struct bpf_d *bd)
d->bd_hlen = bd->bd_hlen;
d->bd_bufsize = bd->bd_bufsize;
d->bd_pid = bd->bd_pid;
- strlcpy(d->bd_ifname,
- bd->bd_bif->bif_ifp->if_xname, IFNAMSIZ);
+ if (bd->bd_bif->bif_ifp != NULL)
+ strlcpy(d->bd_ifname, bd->bd_bif->bif_ifp->if_xname, IFNAMSIZ);
+ else
+ strlcpy(d->bd_ifname, bd->bd_bif->ifname, IFNAMSIZ);
d->bd_locked = bd->bd_locked;
d->bd_wcount = bd->bd_wcount;
d->bd_wdcount = bd->bd_wdcount;
diff --git a/sys/net/bpf.h b/sys/net/bpf.h
index ba2b8ce..808e8a7 100644
--- a/sys/net/bpf.h
+++ b/sys/net/bpf.h
@@ -1226,6 +1226,7 @@ struct bpf_if {
struct rwlock bif_lock; /* interface lock */
LIST_HEAD(, bpf_d) bif_wlist; /* writer-only list */
int flags; /* Interface flags */
+ char *ifname; /* Fake interface name */
#endif
};
@@ -1236,7 +1237,9 @@ void bpf_mtap(struct bpf_if *, struct mbuf *);
void bpf_mtap2(struct bpf_if *, void *, u_int, struct mbuf *);
void bpfattach(struct ifnet *, u_int, u_int);
void bpfattach2(struct ifnet *, u_int, u_int, struct bpf_if **);
+void bpfattach3(char *, u_int, u_int, struct bpf_if **);
void bpfdetach(struct ifnet *);
+void bpfdetach3(char *);
void bpfilterattach(int);
u_int bpf_filter(const struct bpf_insn *, u_char *, u_int, u_int);
diff --git a/sys/netinet/ipfw/ip_fw_log.c b/sys/netinet/ipfw/ip_fw_log.c
index 983fe3b..e1eb817 100644
--- a/sys/netinet/ipfw/ip_fw_log.c
+++ b/sys/netinet/ipfw/ip_fw_log.c
@@ -89,64 +89,28 @@ ipfw_log_bpf(int onoff)
{
}
#else /* !WITHOUT_BPF */
-static struct ifnet *log_if; /* hook to attach to bpf */
-
-/* we use this dummy function for all ifnet callbacks */
-static int
-log_dummy(struct ifnet *ifp, u_long cmd, caddr_t addr)
-{
- return EINVAL;
-}
-
-static int
-ipfw_log_output(struct ifnet *ifp, struct mbuf *m,
- struct sockaddr *dst, struct route *ro)
-{
- if (m != NULL)
- m_freem(m);
- return EINVAL;
-}
-
-static void
-ipfw_log_start(struct ifnet* ifp)
-{
- panic("ipfw_log_start() must not be called");
-}
+static struct bpf_if *log_bpfif = NULL; /* hook to attach to bpf */
+#define BPF_IFNAME "ipfw0"
+#define IPFW_MTAP(_if_bpf,_data,_dlen,_m) do { \
+ if (bpf_peers_present(_if_bpf)) { \
+ M_ASSERTVALID(_m); \
+ bpf_mtap2((_if_bpf),(_data),(_dlen),(_m)); \
+ } \
+} while (0)
static const u_char ipfwbroadcastaddr[6] =
{ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
-
void
ipfw_log_bpf(int onoff)
{
- struct ifnet *ifp;
-
if (onoff) {
- if (log_if)
- return;
- ifp = if_alloc(IFT_ETHER);
- if (ifp == NULL)
+ if (log_bpfif)
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;
+ bpfattach3(BPF_IFNAME, DLT_EN10MB, 14, &log_bpfif);
} else {
- if (log_if) {
- ether_ifdetach(log_if);
- if_free(log_if);
- }
- log_if = NULL;
+ if (log_bpfif != NULL)
+ bpfdetach3(BPF_IFNAME);
+ log_bpfif = NULL;
}
}
#endif /* !WITHOUT_BPF */
@@ -167,16 +131,16 @@ ipfw_log(struct ip_fw *f, u_int hlen, struct ip_fw_args *args,
if (V_fw_verbose == 0) {
#ifndef WITHOUT_BPF
- if (log_if == NULL || log_if->if_bpf == NULL)
+ if (log_bpfif == NULL)
return;
if (args->eh) /* layer2, use orig hdr */
- BPF_MTAP2(log_if, args->eh, ETHER_HDR_LEN, m);
+ IPFW_MTAP(log_bpfif, args->eh, ETHER_HDR_LEN, m);
else
/* Add fake header. Later we will store
* more info in the header.
*/
- BPF_MTAP2(log_if, "DDDDDDSSSSSS\x08\x00", ETHER_HDR_LEN, m);
+ IPFW_MTAP(log_bpfif, "DDDDDDSSSSSS\x08\x00", ETHER_HDR_LEN, m);
#endif /* !WITHOUT_BPF */
return;
}
--------------070804010107020305000604--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4FD4AD29.3040204>