From: Roy Marples <roy@marples.name>
Date: Sun, 17 May 2026 12:12:23 +0000 (+0100)
Subject: BPF: Address review comments
X-Git-Url: http://git.ipfire.org/gitweb/index.cgi?a=commitdiff_plain;h=2f22f3af352ebc746e7ad96c48c736aa8bdbce63;p=thirdparty%2Fdhcpcd.git

BPF: Address review comments
---

diff --git a/src/bpf-bsd.c b/src/bpf-bsd.c
index eecc7e32..5200c12a 100644
--- a/src/bpf-bsd.c
+++ b/src/bpf-bsd.c
@@ -40,7 +40,7 @@
 #include "bpf.h"
 #include "logerr.h"
 
-const char *bpf_name = "Berkley Packet Filter";
+const char *bpf_name = "Berkeley Packet Filter";
 
 struct bpf *
 bpf_open(const struct interface *ifp,
@@ -66,8 +66,6 @@ bpf_open(const struct interface *ifp,
 	bpf = calloc(1, sizeof(*bpf));
 	if (bpf == NULL)
 		return NULL;
-	bpf->bpf_ifp = ifp;
-	bpf->bpf_flags = BPF_EOF;
 
 	/* /dev/bpf is a cloner on modern kernels */
 	bpf->bpf_fd = open("/dev/bpf", BPF_OPEN_FLAGS);
@@ -86,6 +84,9 @@ bpf_open(const struct interface *ifp,
 	if (bpf->bpf_fd == -1)
 		goto eexit;
 
+	bpf->bpf_ifp = ifp;
+	bpf->bpf_flags = BPF_EOF;
+
 #ifndef O_CLOEXEC
 	if ((fd_opts = fcntl(bpf->bpf_fd, F_GETFD)) == -1 ||
 	    fcntl(bpf->bpf_fd, F_SETFD, fd_opts | FD_CLOEXEC) == -1)
@@ -125,9 +126,7 @@ bpf_open(const struct interface *ifp,
 	return bpf;
 
 eexit:
-	if (bpf->bpf_fd != -1)
-		close(bpf->bpf_fd);
-	free(bpf);
+	bpf_close(bpf);
 	return NULL;
 }
 
@@ -138,54 +137,67 @@ bpf_read(struct bpf *bpf, void *data, size_t len)
 {
 	ssize_t bytes;
 	struct bpf_hdr packet;
-	const char *payload;
+	size_t hdr_max;
+	const uint8_t *payload;
 
 	bpf->bpf_flags &= ~BPF_EOF;
-	for (;;) {
-		if (bpf->bpf_len == 0) {
-			bytes = read(bpf->bpf_fd, bpf->bpf_buffer,
-			    bpf->bpf_size);
-#if defined(__sun)
-			/* After 2^31 bytes, the kernel offset overflows.
-			 * To work around this bug, lseek 0. */
-			if (bytes == -1 && errno == EINVAL) {
-				lseek(bpf->bpf_fd, 0, SEEK_SET);
-				continue;
-			}
-#endif
-			if (bytes == -1 || bytes == 0)
-				return bytes;
-			bpf->bpf_len = (size_t)bytes;
-			bpf->bpf_pos = 0;
+	if (bpf->bpf_len == 0) {
+		bytes = read(bpf->bpf_fd, bpf->bpf_buffer, bpf->bpf_size);
+#ifdef __sun
+		/* After 2^31 bytes, the kernel offset overflows.
+		 * To work around this bug, lseek 0. */
+		if (bytes == -1 && errno == EINVAL) {
+			lseek(bpf->bpf_fd, 0, SEEK_SET);
+			return 0;
 		}
-		bytes = -1;
-		payload = (const char *)bpf->bpf_buffer + bpf->bpf_pos;
-		memcpy(&packet, payload, sizeof(packet));
-		if (bpf->bpf_pos + packet.bh_caplen + packet.bh_hdrlen >
-		    bpf->bpf_len)
-			goto next; /* Packet beyond buffer, drop. */
-		payload += packet.bh_hdrlen;
-		if (packet.bh_caplen > len)
-			bytes = (ssize_t)len;
-		else
-			bytes = (ssize_t)packet.bh_caplen;
-		if (bpf_frame_bcast(bpf->bpf_ifp, payload) == 0)
-			bpf->bpf_flags |= BPF_BCAST;
-		else
-			bpf->bpf_flags &= ~BPF_BCAST;
-		memcpy(data, payload, (size_t)bytes);
-	next:
-		bpf->bpf_pos += BPF_WORDALIGN(
-		    packet.bh_hdrlen + packet.bh_caplen);
-		if (bpf->bpf_pos >= bpf->bpf_len) {
-			bpf->bpf_len = bpf->bpf_pos = 0;
-			bpf->bpf_flags |= BPF_EOF;
-		}
-		if (bytes != -1)
+#endif
+		if (bytes == -1 || bytes == 0)
 			return bytes;
+		bpf->bpf_len = (size_t)bytes;
+		bpf->bpf_pos = 0;
+	}
+
+	if (bpf->bpf_pos + sizeof(packet) > bpf->bpf_len) {
+		errno = EINVAL;
+		goto err;
+	}
+
+	payload = (const uint8_t *)bpf->bpf_buffer + bpf->bpf_pos;
+	memcpy(&packet, payload, sizeof(packet));
+
+	hdr_max = SIZE_MAX - packet.bh_caplen;
+	if (packet.bh_hdrlen > hdr_max) {
+		errno = EOVERFLOW;
+		goto err;
+	}
+	if (packet.bh_hdrlen + packet.bh_caplen > bpf->bpf_len - bpf->bpf_pos) {
+		errno = EBADMSG;
+		goto err;
+	}
+
+	payload += packet.bh_hdrlen;
+	if (packet.bh_caplen > len)
+		bytes = (ssize_t)len;
+	else
+		bytes = (ssize_t)packet.bh_caplen;
+
+	if (bpf_frame_bcast(bpf->bpf_ifp, payload) == 0)
+		bpf->bpf_flags |= BPF_BCAST;
+	else
+		bpf->bpf_flags &= ~BPF_BCAST;
+	memcpy(data, payload, (size_t)bytes);
+
+	bpf->bpf_pos += BPF_WORDALIGN(packet.bh_hdrlen + packet.bh_caplen);
+	if (bpf->bpf_pos >= bpf->bpf_len) {
+		bpf->bpf_len = bpf->bpf_pos = 0;
+		bpf->bpf_flags |= BPF_EOF;
 	}
+	return bytes;
 
-	/* NOTREACHED */
+err:
+	bpf->bpf_len = bpf->bpf_pos = 0;
+	bpf->bpf_flags |= BPF_EOF;
+	return -1;
 }
 
 int
@@ -239,7 +251,8 @@ bpf_writev(const struct bpf *bpf, struct iovec *iov, int iovcnt)
 void
 bpf_close(struct bpf *bpf)
 {
-	close(bpf->bpf_fd);
+	if (bpf->bpf_fd != -1)
+		close(bpf->bpf_fd);
 	free(bpf->bpf_buffer);
 	free(bpf);
 }
diff --git a/src/bpf-linux.c b/src/bpf-linux.c
index 1a374ec1..aa0be5b4 100644
--- a/src/bpf-linux.c
+++ b/src/bpf-linux.c
@@ -55,6 +55,7 @@ bpf_open(const struct interface *ifp,
 		     .sll_protocol = htons(ETH_P_ALL),
 		     .sll_ifindex = (int)ifp->index,
 		 } };
+	int mtu;
 #ifdef PACKET_AUXDATA
 	int n;
 #endif
@@ -64,9 +65,10 @@ bpf_open(const struct interface *ifp,
 		return NULL;
 	bpf->bpf_ifp = ifp;
 	bpf->bpf_flags = BPF_EOF;
+	bpf->bpf_fd = -1;
 
-	/* Allocate a suitably large buffer for a single packet. */
-	bpf->bpf_size = ETH_FRAME_LEN;
+	mtu = ifp->mtu ? ifp->mtu : ETH_DATA_LEN;
+	bpf->bpf_size = bpf_frame_header_len(ifp) + (size_t)mtu;
 	bpf->bpf_buffer = malloc(bpf->bpf_size);
 	if (bpf->bpf_buffer == NULL)
 		goto eexit;
@@ -106,10 +108,7 @@ bpf_open(const struct interface *ifp,
 	return bpf;
 
 eexit:
-	if (bpf->bpf_fd != -1)
-		close(bpf->bpf_fd);
-	free(bpf->bpf_buffer);
-	free(bpf);
+	bpf_close(bpf);
 	return NULL;
 }
 
@@ -141,6 +140,10 @@ bpf_read(struct bpf *bpf, void *data, size_t len)
 	bytes = recvmsg(bpf->bpf_fd, &msg, 0);
 	if (bytes == -1)
 		return -1;
+	if (msg.msg_flags & MSG_TRUNC) {
+		errno = ENOBUFS;
+		return -1;
+	}
 	bpf->bpf_flags |= BPF_EOF; /* We only ever read one packet. */
 	bpf->bpf_flags &= ~BPF_PARTIALCSUM;
 	if (bytes) {
@@ -211,7 +214,8 @@ bpf_writev(const struct bpf *bpf, struct iovec *iov, int iovcnt)
 void
 bpf_close(struct bpf *bpf)
 {
-	close(bpf->bpf_fd);
+	if (bpf->bpf_fd != -1)
+		close(bpf->bpf_fd);
 	free(bpf->bpf_buffer);
 	free(bpf);
 }
diff --git a/src/bpf-pcap.c b/src/bpf-pcap.c
index 93d294af..931037a5 100644
--- a/src/bpf-pcap.c
+++ b/src/bpf-pcap.c
@@ -47,7 +47,7 @@
 
 #define ETH_MTU 1500
 
-const char *bpf_name = "Berkley Packet Filter (libpcap)";
+const char *bpf_name = "Berkeley Packet Filter (libpcap)";
 
 struct bpf *
 bpf_open(const struct interface *ifp,
@@ -58,15 +58,15 @@ bpf_open(const struct interface *ifp,
 	struct bpf *bpf;
 	pcap_t *handle;
 	char errbuf[PCAP_ERRBUF_SIZE];
-	int snaplen;
+	int mtu;
 
 	bpf = calloc(1, sizeof(*bpf));
 	if (bpf == NULL)
 		return NULL;
 
-	snaplen = ifp->mtu ? ifp->mtu : ETH_MTU;
+	mtu = ifp->mtu ? ifp->mtu : ETH_MTU;
 	bpf->bpf_ifp = ifp;
-	bpf->bpf_size = bpf_frame_header_len(ifp) + (size_t)snaplen;
+	bpf->bpf_size = bpf_frame_header_len(ifp) + (size_t)mtu;
 	bpf->bpf_buffer = malloc(bpf->bpf_size);
 	if (bpf->bpf_buffer == NULL)
 		goto eexit;
@@ -80,7 +80,7 @@ bpf_open(const struct interface *ifp,
 		goto eexit;
 	}
 
-	PCAP_CHECK(pcap_set_snaplen(handle, snaplen), "pcap_set_snaplen");
+	PCAP_CHECK(pcap_set_snaplen(handle, (int)bpf->bpf_size), "pcap_set_snaplen");
 	PCAP_CHECK(pcap_set_promisc(handle, 0), "pcap_set_promisc");
 	PCAP_CHECK(pcap_set_immediate_mode(handle, 1),
 	    "pcap_set_immediate_mode");
@@ -124,6 +124,8 @@ bpf_read(struct bpf *bpf, void *data, size_t len)
 
 	err = pcap_next_ex(bpf->bpf_handle, &pkt_header, &pkt_data);
 
+	if (err == 0)
+		return 0;
 	if (err < 0)
 		return -1;
 
@@ -133,6 +135,11 @@ bpf_read(struct bpf *bpf, void *data, size_t len)
 		cap_len = len;
 	memcpy(data, pkt_data, cap_len);
 
+	if (bpf_frame_bcast(bpf->bpf_ifp, pkt_data) == 0)
+		bpf->bpf_flags |= BPF_BCAST;
+	else
+		bpf->bpf_flags &= ~BPF_BCAST;
+
 	return (ssize_t)cap_len;
 }
 
@@ -144,17 +151,23 @@ bpf_writev(const struct bpf *bpf, struct iovec *iov, int iovcnt)
 	uint8_t *bp = bpf->bpf_buffer;
 
 	for (i = 0; i < iovcnt; i++) {
-		len += iov[i].iov_len;
 		/* This should be impossible. */
-		if (bpf->bpf_size < len) {
+		if (iov[i].iov_len > bpf->bpf_size - len) {
 			errno = ENOBUFS;
 			return -1;
 		}
+
 		memcpy(bp, iov[i].iov_base, iov[i].iov_len);
 		bp += iov[i].iov_len;
+		len += iov[i].iov_len;
 	}
 
-	return pcap_inject(bpf->bpf_handle, bpf->bpf_buffer, len);
+	i = pcap_inject(bpf->bpf_handle, bpf->bpf_buffer, len);
+	if (i < 0) {
+		logerrx("%s: %s", __func__, pcap_geterr(bpf->bpf_handle));
+		return -1;
+	}
+	return i;
 }
 
 int
diff --git a/src/bpf.c b/src/bpf.c
index 2cccc5f7..20032c9d 100644
--- a/src/bpf.c
+++ b/src/bpf.c
@@ -363,9 +363,9 @@ bpf_arp_rw(const struct bpf *bpf, const struct in_addr *ia, bool recv)
 	bp++;
 
 	len = (unsigned int)(bp - buf);
-	if (recv) return bpf_setfilter(bpf, buf, len);
+	if (recv)
+		return bpf_setfilter(bpf, buf, len);
 	return bpf_setwfilter(bpf, buf, len);
-		return -1;
 }
 
 int
@@ -441,7 +441,7 @@ static const struct bpf_insn bpf_bootp_write[] = {
 	BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, (BOOTPC << 16) + BOOTPS, 1, 0),
 	BPF_STMT(BPF_RET + BPF_K, 0),
 };
-#define BPF_BOOTP_WRITE_LEN __arraycount(bpf_bootp_write)
+#define BPF_BOOTP_WRITE_LEN  __arraycount(bpf_bootp_write)
 
 #define BPF_BOOTP_CHADDR_LEN ((BOOTP_CHADDR_LEN / 4) * 3)
 #define BPF_BOOTP_XID_LEN    4 /* BOUND check is 4 instructions */
@@ -503,7 +503,7 @@ int
 bpf_filter_bootp(const struct bpf *bpf, __unused const struct in_addr *ia)
 {
 	if (bpf_bootp_rw(bpf, true) == -1)
-	       return -1;
+		return -1;
 	if (bpf_bootp_rw(bpf, false) == -1 && errno != ENOSYS)
 		return -1;
 	if (bpf_lock(bpf) == -1 && errno != ENOSYS)