BPF: Address review comments

diff --git a/src/bpf-bsd.c b/src/bpf-bsd.c
index eecc7e32288d075a52fe09ce0b6b517fc306d66f..5200c12a070eab30ab8cc984bdb5316f88696ac3 100644 (file)
--- 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 1a374ec1829e08993ab583f53cc8640fcf093851..aa0be5b4934ece72749818f3ef75fbdee91fbf63 100644 (file)
--- 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 93d294af71568a60d82b07e87fa663334db28e0a..931037a5e971a956cba891c6e6e945642aa921c6 100644 (file)
--- 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 2cccc5f70fe0041dcd29ca97bf0b174f54c50ef3..20032c9d4daef627d3e2a3dd6ac99bc428c9a0f8 100644 (file)
--- 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)