Simplify and clarify (i.e. shrink) code processing

the bpf captures.

3 files changed, 85 insertions, 129 deletions

diff --git a/sbin/dhclient/bpf.c b/sbin/dhclient/bpf.c
index 09b4cde9574..d1aeeaf8ba7 100644
--- a/sbin/dhclient/bpf.c
+++ b/sbin/dhclient/bpf.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: bpf.c,v 1.73 2018/12/27 17:33:15 krw Exp $	*/
+/*	$OpenBSD: bpf.c,v 1.74 2019/01/05 21:40:44 krw Exp $	*/
 
 /* BPF socket interface code, originally contributed by Archie Cobbs. */
 
@@ -330,123 +330,71 @@ send_packet(struct interface_info *ifi, struct in_addr from, struct in_addr to,
 	return result;
 }
 
+/*
+ * Extract a DHCP packet from a bpf capture buffer.
+ *
+ * Each captured packet is
+ *
+ *	<BPF header>
+ *	<padding to BPF_WORDALIGN>
+ *	<captured DHCP packet>
+ *	<padding to BPF_WORDALIGN>
+ *
+ * Return the number of bytes processed or 0 if there is
+ * no valid DHCP packet in the buffer.
+ */
 ssize_t
-receive_packet(struct interface_info *ifi, struct sockaddr_in *from,
-    struct ether_addr *hfrom)
+receive_packet(unsigned char *buf, unsigned char *lim,
+    struct sockaddr_in *from, struct ether_addr *hfrom,
+    struct dhcp_packet *packet)
 {
-	struct bpf_hdr		 hdr;
-	struct dhcp_packet	*packet = &ifi->recv_packet;
-	ssize_t			 length = 0;
-	int			 offset = 0;
-
-	/*
-	 * All this complexity is because BPF doesn't guarantee that
-	 * only one packet will be returned at a time.  We're getting
-	 * what we deserve, though - this is a terrible abuse of the BPF
-	 * interface.  Sigh.
-	 */
-
-	/* Process packets until we get one we can return or until we've
-	 * done a read and gotten nothing we can return.
-	 */
-	do {
-		/* If the buffer is empty, fill it. */
-		if (ifi->rbuf_offset >= ifi->rbuf_len) {
-			length = read(ifi->bpffd, ifi->rbuf, ifi->rbuf_max);
-			if (length == -1) {
-				log_warn("%s: read(bpffd)", log_procname);
-				return length;
-			} else if (length == 0)
-				return length;
-			ifi->rbuf_offset = 0;
-			ifi->rbuf_len = length;
-		}
+	struct bpf_hdr		 bh;
+	struct ether_header	 eh;
+	unsigned char		*pktlim, *data, *next;
+	size_t			 datalen;
+	int			 len;
 
-		/*
-		 * If there isn't room for a whole bpf header, something
-		 * went wrong, but we'll ignore it and hope it goes
-		 * away. XXX
-		 */
-		if (ifi->rbuf_len - ifi->rbuf_offset < sizeof(hdr)) {
-			ifi->rbuf_offset = ifi->rbuf_len;
-			continue;
-		}
+	for (next = buf; next < lim; next = pktlim) {
+		/* No bpf header means no more packets. */
+		if (lim < next + sizeof(bh))
+			return 0;
 
-		/* Copy out a bpf header. */
-		memcpy(&hdr, &ifi->rbuf[ifi->rbuf_offset], sizeof(hdr));
+		memcpy(&bh, next, sizeof(bh));
+		pktlim = next + BPF_WORDALIGN(bh.bh_hdrlen + bh.bh_caplen);
 
-		/*
-		 * If the bpf header plus data doesn't fit in what's
-		 * left of the buffer, stick head in sand yet again.
-		 */
-		if (ifi->rbuf_offset + hdr.bh_hdrlen + hdr.bh_caplen >
-		    ifi->rbuf_len) {
-			ifi->rbuf_offset = ifi->rbuf_len;
-			continue;
-		}
+		/* Truncated bpf packet means no more packets. */
+		if (lim < next + bh.bh_hdrlen + bh.bh_caplen)
+			return 0;
 
-		/*
-		 * If the captured data wasn't the whole packet, or if
-		 * the packet won't fit in the input buffer, all we can
-		 * do is drop it.
-		 */
-		if (hdr.bh_caplen != hdr.bh_datalen) {
-			ifi->rbuf_offset = BPF_WORDALIGN(
-			    ifi->rbuf_offset + hdr.bh_hdrlen +
-			    hdr.bh_caplen);
+		/* Drop incompletely captured DHCP packets. */
+		if (bh.bh_caplen != bh.bh_datalen)
 			continue;
-		}
-
-		/* Skip over the BPF header. */
-		ifi->rbuf_offset += hdr.bh_hdrlen;
-
-		/* Decode the physical header. */
-		offset = decode_hw_header(ifi->rbuf + ifi->rbuf_offset,
-		    hdr.bh_caplen, hfrom);
 
 		/*
-		 * If a physical layer checksum failed (dunno of any
-		 * physical layer that supports this, but WTH), skip
-		 * this packet.
+		 * Drop packets with invalid ethernet/ip/udp headers.
 		 */
-		if (offset < 0) {
-			ifi->rbuf_offset = BPF_WORDALIGN(
-			    ifi->rbuf_offset + hdr.bh_caplen);
+		if (pktlim < next + bh.bh_hdrlen + sizeof(eh))
 			continue;
-		}
-		ifi->rbuf_offset += offset;
-		hdr.bh_caplen -= offset;
+		memcpy(&eh, next + bh.bh_hdrlen, sizeof(eh));
+		memcpy(hfrom->ether_addr_octet, eh.ether_shost, ETHER_ADDR_LEN);
 
-		/* Decode the IP and UDP headers. */
-		offset = decode_udp_ip_header(ifi->rbuf + ifi->rbuf_offset,
-		    hdr.bh_caplen, from);
-
-		/* If the IP or UDP checksum was bad, skip the packet. */
-		if (offset < 0) {
-			ifi->rbuf_offset = BPF_WORDALIGN(
-			    ifi->rbuf_offset + hdr.bh_caplen);
+		len = decode_udp_ip_header(next + bh.bh_hdrlen + sizeof(eh),
+		    bh.bh_caplen - sizeof(eh), from);
+		if (len < 0)
 			continue;
-		}
-		ifi->rbuf_offset += offset;
-		hdr.bh_caplen -= offset;
 
-		/*
-		 * If there's not enough room to stash the packet data,
-		 * we have to skip it (this shouldn't happen in real
-		 * life, though).
-		 */
-		if (hdr.bh_caplen > sizeof(*packet)) {
-			ifi->rbuf_offset = BPF_WORDALIGN(
-			    ifi->rbuf_offset + hdr.bh_caplen);
+		/* Drop packets larger than sizeof(struct dhcp_packet). */
+		datalen = bh.bh_caplen - (sizeof(eh) + len);
+		if (datalen > sizeof(*packet))
 			continue;
-		}
 
-		/* Copy out the data in the packet. */
+		/* Extract the DHCP packet for further processing. */
+		data = next + bh.bh_hdrlen + sizeof(eh) + len;
 		memset(packet, DHO_END, sizeof(*packet));
-		memcpy(packet, ifi->rbuf + ifi->rbuf_offset, hdr.bh_caplen);
-		ifi->rbuf_offset = BPF_WORDALIGN(ifi->rbuf_offset +
-		    hdr.bh_caplen);
-		return hdr.bh_caplen;
-	} while (length == 0);
-	return  0 ;
+		memcpy(packet, data, datalen);
+
+		return (pktlim - buf);
+	}
+
+	return 0;
 }
diff --git a/sbin/dhclient/dhcpd.h b/sbin/dhclient/dhcpd.h
index c2874e89c10..e092f13ab00 100644
--- a/sbin/dhclient/dhcpd.h
+++ b/sbin/dhclient/dhcpd.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: dhcpd.h,v 1.262 2019/01/03 16:42:30 krw Exp $	*/
+/*	$OpenBSD: dhcpd.h,v 1.263 2019/01/05 21:40:44 krw Exp $	*/
 
 /*
  * Copyright (c) 2004 Henning Brauer <henning@openbsd.org>
@@ -123,8 +123,6 @@ struct interface_info {
 	int			 udpfd; /* udp - unicast writing */
 	unsigned char		*rbuf;
 	size_t			 rbuf_max;
-	size_t			 rbuf_offset;
-	size_t			 rbuf_len;
 	int			 errors;
 	uint16_t		 index;
 	int			 link_state;
@@ -193,8 +191,8 @@ int		 get_udp_sock(int);
 int		 configure_bpf_sock(int);
 ssize_t		 send_packet(struct interface_info *, struct in_addr,
     struct in_addr, const char *);
-ssize_t		 receive_packet(struct interface_info *, struct sockaddr_in *,
-    struct ether_addr *);
+ssize_t		 receive_packet(unsigned char *, unsigned char *,
+    struct sockaddr_in *, struct ether_addr *, struct dhcp_packet *);
 
 /* dispatch.c */
 void		 dispatch(struct interface_info *, int);
diff --git a/sbin/dhclient/dispatch.c b/sbin/dhclient/dispatch.c
index 3d519d40afe..03e9619c59f 100644
--- a/sbin/dhclient/dispatch.c
+++ b/sbin/dhclient/dispatch.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: dispatch.c,v 1.157 2019/01/05 19:59:12 krw Exp $	*/
+/*	$OpenBSD: dispatch.c,v 1.158 2019/01/05 21:40:44 krw Exp $	*/
 
 /*
  * Copyright 2004 Henning Brauer <henning@openbsd.org>
@@ -73,6 +73,8 @@
 
 
 void bpffd_handler(struct interface_info *);
+void dhcp_packet_dispatch(struct interface_info *, struct sockaddr_in *,
+    struct ether_addr *);
 void flush_unpriv_ibuf(void);
 void sendhup(void);
 
@@ -167,12 +169,8 @@ dispatch(struct interface_info *ifi, int routefd)
 		if (nfds == 0)
 			continue;
 
-		if ((fds[0].revents & POLLIN) != 0) {
-			ifi->rbuf_offset = ifi->rbuf_len = 0;
-			do {
-				bpffd_handler(ifi);
-			} while (quit == 0 && ifi->rbuf_offset < ifi->rbuf_len);
-		}
+		if ((fds[0].revents & POLLIN) != 0)
+			bpffd_handler(ifi);
 		if ((fds[1].revents & POLLIN) != 0)
 			routefd_handler(ifi, routefd);
 		if ((fds[2].revents & POLLOUT) != 0)
@@ -190,27 +188,39 @@ bpffd_handler(struct interface_info *ifi)
 {
 	struct sockaddr_in	 from;
 	struct ether_addr	 hfrom;
-	struct in_addr		 ifrom;
-	struct dhcp_packet	*packet = &ifi->recv_packet;
-	struct reject_elem	*ap;
-	struct option_data	*options;
-	char			*src;
-	ssize_t			 result;
-	int			 i, rslt;
+	unsigned char		*next, *lim;
+	ssize_t			 n;
 
-	result = receive_packet(ifi, &from, &hfrom);
-	if (result == -1) {
+	n = read(ifi->bpffd, ifi->rbuf, ifi->rbuf_max);
+	if (n == -1) {
+		log_warn("%s: read(bpffd)", log_procname);
 		ifi->errors++;
 		if (ifi->errors > 20)
-			fatalx("too many receive_packet failures");
+			fatalx("too many read(bpffd) failures");
 		return;
 	}
 	ifi->errors = 0;
 
-	if (result == 0)
-		return;
+	lim = ifi->rbuf + n;
+	for (next = ifi->rbuf; quit == 0 && n > 0; next += n) {
+		n = receive_packet(next, lim, &from, &hfrom, &ifi->recv_packet);
+		if (n > 0)
+			dhcp_packet_dispatch(ifi, &from, &hfrom);
+	}
+}
+
+void
+dhcp_packet_dispatch(struct interface_info *ifi, struct sockaddr_in *from,
+    struct ether_addr *hfrom)
+{
+	struct in_addr		 ifrom;
+	struct dhcp_packet	*packet = &ifi->recv_packet;
+	struct reject_elem	*ap;
+	struct option_data	*options;
+	char			*src;
+	int			 i, rslt;
 
-	ifrom.s_addr = from.sin_addr.s_addr;
+	ifrom.s_addr = from->sin_addr.s_addr;
 
 	if (packet->hlen != ETHER_ADDR_LEN) {
 		log_debug("%s: discarding packet with hlen == %u", log_procname,
@@ -253,7 +263,7 @@ bpffd_handler(struct interface_info *ifi)
 		return;
 	}
 
-	rslt = asprintf(&src, "%s (%s)", inet_ntoa(ifrom), ether_ntoa(&hfrom));
+	rslt = asprintf(&src, "%s (%s)", inet_ntoa(ifrom), ether_ntoa(hfrom));
 	if (rslt == -1)
 		fatal("src");