Skip to content

Commit 2bedcda

Browse files
rsmarplesrsmarples
committed
BPF: Address review comments
1 parent 6ae0742 commit 2bedcda

4 files changed

Lines changed: 94 additions & 64 deletions

File tree

‎src/bpf-bsd.c‎

Lines changed: 62 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@
4040
#include "bpf.h"
4141
#include "logerr.h"
4242

43-
const char *bpf_name = "Berkley Packet Filter";
43+
const char *bpf_name = "Berkeley Packet Filter";
4444

4545
struct bpf *
4646
bpf_open(const struct interface *ifp,
@@ -66,8 +66,6 @@ bpf_open(const struct interface *ifp,
6666
bpf = calloc(1, sizeof(*bpf));
6767
if (bpf == NULL)
6868
return NULL;
69-
bpf->bpf_ifp = ifp;
70-
bpf->bpf_flags = BPF_EOF;
7169

7270
/* /dev/bpf is a cloner on modern kernels */
7371
bpf->bpf_fd = open("/dev/bpf", BPF_OPEN_FLAGS);
@@ -86,6 +84,9 @@ bpf_open(const struct interface *ifp,
8684
if (bpf->bpf_fd == -1)
8785
goto eexit;
8886

87+
bpf->bpf_ifp = ifp;
88+
bpf->bpf_flags = BPF_EOF;
89+
8990
#ifndef O_CLOEXEC
9091
if ((fd_opts = fcntl(bpf->bpf_fd, F_GETFD)) == -1 ||
9192
fcntl(bpf->bpf_fd, F_SETFD, fd_opts | FD_CLOEXEC) == -1)
@@ -125,9 +126,7 @@ bpf_open(const struct interface *ifp,
125126
return bpf;
126127

127128
eexit:
128-
if (bpf->bpf_fd != -1)
129-
close(bpf->bpf_fd);
130-
free(bpf);
129+
bpf_close(bpf);
131130
return NULL;
132131
}
133132

@@ -138,54 +137,67 @@ bpf_read(struct bpf *bpf, void *data, size_t len)
138137
{
139138
ssize_t bytes;
140139
struct bpf_hdr packet;
141-
const char *payload;
140+
size_t hdr_max;
141+
const uint8_t *payload;
142142

143143
bpf->bpf_flags &= ~BPF_EOF;
144-
for (;;) {
145-
if (bpf->bpf_len == 0) {
146-
bytes = read(bpf->bpf_fd, bpf->bpf_buffer,
147-
bpf->bpf_size);
148-
#if defined(__sun)
149-
/* After 2^31 bytes, the kernel offset overflows.
150-
* To work around this bug, lseek 0. */
151-
if (bytes == -1 && errno == EINVAL) {
152-
lseek(bpf->bpf_fd, 0, SEEK_SET);
153-
continue;
154-
}
155-
#endif
156-
if (bytes == -1 || bytes == 0)
157-
return bytes;
158-
bpf->bpf_len = (size_t)bytes;
159-
bpf->bpf_pos = 0;
144+
if (bpf->bpf_len == 0) {
145+
bytes = read(bpf->bpf_fd, bpf->bpf_buffer, bpf->bpf_size);
146+
#ifdef __sun
147+
/* After 2^31 bytes, the kernel offset overflows.
148+
* To work around this bug, lseek 0. */
149+
if (bytes == -1 && errno == EINVAL) {
150+
lseek(bpf->bpf_fd, 0, SEEK_SET);
151+
return 0;
160152
}
161-
bytes = -1;
162-
payload = (const char *)bpf->bpf_buffer + bpf->bpf_pos;
163-
memcpy(&packet, payload, sizeof(packet));
164-
if (bpf->bpf_pos + packet.bh_caplen + packet.bh_hdrlen >
165-
bpf->bpf_len)
166-
goto next; /* Packet beyond buffer, drop. */
167-
payload += packet.bh_hdrlen;
168-
if (packet.bh_caplen > len)
169-
bytes = (ssize_t)len;
170-
else
171-
bytes = (ssize_t)packet.bh_caplen;
172-
if (bpf_frame_bcast(bpf->bpf_ifp, payload) == 0)
173-
bpf->bpf_flags |= BPF_BCAST;
174-
else
175-
bpf->bpf_flags &= ~BPF_BCAST;
176-
memcpy(data, payload, (size_t)bytes);
177-
next:
178-
bpf->bpf_pos += BPF_WORDALIGN(
179-
packet.bh_hdrlen + packet.bh_caplen);
180-
if (bpf->bpf_pos >= bpf->bpf_len) {
181-
bpf->bpf_len = bpf->bpf_pos = 0;
182-
bpf->bpf_flags |= BPF_EOF;
183-
}
184-
if (bytes != -1)
153+
#endif
154+
if (bytes == -1 || bytes == 0)
185155
return bytes;
156+
bpf->bpf_len = (size_t)bytes;
157+
bpf->bpf_pos = 0;
158+
}
159+
160+
if (bpf->bpf_pos + sizeof(packet) > bpf->bpf_len) {
161+
errno = EINVAL;
162+
goto err;
163+
}
164+
165+
payload = (const uint8_t *)bpf->bpf_buffer + bpf->bpf_pos;
166+
memcpy(&packet, payload, sizeof(packet));
167+
168+
hdr_max = SIZE_MAX - packet.bh_caplen;
169+
if (packet.bh_hdrlen > hdr_max) {
170+
errno = EOVERFLOW;
171+
goto err;
172+
}
173+
if (packet.bh_hdrlen + packet.bh_caplen > bpf->bpf_len - bpf->bpf_pos) {
174+
errno = EBADMSG;
175+
goto err;
176+
}
177+
178+
payload += packet.bh_hdrlen;
179+
if (packet.bh_caplen > len)
180+
bytes = (ssize_t)len;
181+
else
182+
bytes = (ssize_t)packet.bh_caplen;
183+
184+
if (bpf_frame_bcast(bpf->bpf_ifp, payload) == 0)
185+
bpf->bpf_flags |= BPF_BCAST;
186+
else
187+
bpf->bpf_flags &= ~BPF_BCAST;
188+
memcpy(data, payload, (size_t)bytes);
189+
190+
bpf->bpf_pos += BPF_WORDALIGN(packet.bh_hdrlen + packet.bh_caplen);
191+
if (bpf->bpf_pos >= bpf->bpf_len) {
192+
bpf->bpf_len = bpf->bpf_pos = 0;
193+
bpf->bpf_flags |= BPF_EOF;
186194
}
195+
return bytes;
187196

188-
/* NOTREACHED */
197+
err:
198+
bpf->bpf_len = bpf->bpf_pos = 0;
199+
bpf->bpf_flags |= BPF_EOF;
200+
return -1;
189201
}
190202

191203
int
@@ -239,7 +251,8 @@ bpf_writev(const struct bpf *bpf, struct iovec *iov, int iovcnt)
239251
void
240252
bpf_close(struct bpf *bpf)
241253
{
242-
close(bpf->bpf_fd);
254+
if (bpf->bpf_fd != -1)
255+
close(bpf->bpf_fd);
243256
free(bpf->bpf_buffer);
244257
free(bpf);
245258
}

‎src/bpf-linux.c‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ bpf_open(const struct interface *ifp,
5555
.sll_protocol = htons(ETH_P_ALL),
5656
.sll_ifindex = (int)ifp->index,
5757
} };
58+
size_t mtu;
5859
#ifdef PACKET_AUXDATA
5960
int n;
6061
#endif
@@ -64,9 +65,10 @@ bpf_open(const struct interface *ifp,
6465
return NULL;
6566
bpf->bpf_ifp = ifp;
6667
bpf->bpf_flags = BPF_EOF;
68+
bpf->bpf_fd = -1;
6769

68-
/* Allocate a suitably large buffer for a single packet. */
69-
bpf->bpf_size = ETH_FRAME_LEN;
70+
mtu = ifp->mtu ? ifp->mtu : ETH_DATA_LEN;
71+
bpf->bpf_size = bpf_frame_header_len(ifp) + mtu;
7072
bpf->bpf_buffer = malloc(bpf->bpf_size);
7173
if (bpf->bpf_buffer == NULL)
7274
goto eexit;
@@ -106,10 +108,7 @@ bpf_open(const struct interface *ifp,
106108
return bpf;
107109

108110
eexit:
109-
if (bpf->bpf_fd != -1)
110-
close(bpf->bpf_fd);
111-
free(bpf->bpf_buffer);
112-
free(bpf);
111+
bpf_close(bpf);
113112
return NULL;
114113
}
115114

@@ -141,6 +140,10 @@ bpf_read(struct bpf *bpf, void *data, size_t len)
141140
bytes = recvmsg(bpf->bpf_fd, &msg, 0);
142141
if (bytes == -1)
143142
return -1;
143+
if (msg.msg_flags & MSG_TRUNC) {
144+
errno = ENOBUFS;
145+
return -1;
146+
}
144147
bpf->bpf_flags |= BPF_EOF; /* We only ever read one packet. */
145148
bpf->bpf_flags &= ~BPF_PARTIALCSUM;
146149
if (bytes) {
@@ -211,7 +214,8 @@ bpf_writev(const struct bpf *bpf, struct iovec *iov, int iovcnt)
211214
void
212215
bpf_close(struct bpf *bpf)
213216
{
214-
close(bpf->bpf_fd);
217+
if (bpf->bpf_fd != -1)
218+
close(bpf->bpf_fd);
215219
free(bpf->bpf_buffer);
216220
free(bpf);
217221
}

‎src/bpf-pcap.c‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@
4747

4848
#define ETH_MTU 1500
4949

50-
const char *bpf_name = "Berkley Packet Filter (libpcap)";
50+
const char *bpf_name = "Berkeley Packet Filter (libpcap)";
5151

5252
struct bpf *
5353
bpf_open(const struct interface *ifp,
@@ -124,6 +124,8 @@ bpf_read(struct bpf *bpf, void *data, size_t len)
124124

125125
err = pcap_next_ex(bpf->bpf_handle, &pkt_header, &pkt_data);
126126

127+
if (err == 0)
128+
return 0;
127129
if (err < 0)
128130
return -1;
129131

@@ -133,6 +135,11 @@ bpf_read(struct bpf *bpf, void *data, size_t len)
133135
cap_len = len;
134136
memcpy(data, pkt_data, cap_len);
135137

138+
if (bpf_frame_bcast(bpf->bpf_ifp, pkt_data) == 0)
139+
bpf->bpf_flags |= BPF_BCAST;
140+
else
141+
bpf->bpf_flags &= ~BPF_BCAST;
142+
136143
return (ssize_t)cap_len;
137144
}
138145

@@ -144,17 +151,23 @@ bpf_writev(const struct bpf *bpf, struct iovec *iov, int iovcnt)
144151
uint8_t *bp = bpf->bpf_buffer;
145152

146153
for (i = 0; i < iovcnt; i++) {
147-
len += iov[i].iov_len;
148154
/* This should be impossible. */
149-
if (bpf->bpf_size < len) {
155+
if (iov[i].iov_len > bpf->bpf_size - len) {
150156
errno = ENOBUFS;
151157
return -1;
152158
}
159+
153160
memcpy(bp, iov[i].iov_base, iov[i].iov_len);
154161
bp += iov[i].iov_len;
162+
len += iov[i].iov_len;
155163
}
156164

157-
return pcap_inject(bpf->bpf_handle, bpf->bpf_buffer, len);
165+
i = pcap_inject(bpf->bpf_handle, bpf->bpf_buffer, len);
166+
if (i < 0) {
167+
logerrx("%s: %s", __func__, pcap_geterr(bpf->bpf_handle));
168+
return -1;
169+
}
170+
return i;
158171
}
159172

160173
int

‎src/bpf.c‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -363,9 +363,9 @@ bpf_arp_rw(const struct bpf *bpf, const struct in_addr *ia, bool recv)
363363
bp++;
364364

365365
len = (unsigned int)(bp - buf);
366-
if (recv) return bpf_setfilter(bpf, buf, len);
366+
if (recv)
367+
return bpf_setfilter(bpf, buf, len);
367368
return bpf_setwfilter(bpf, buf, len);
368-
return -1;
369369
}
370370

371371
int
@@ -441,7 +441,7 @@ static const struct bpf_insn bpf_bootp_write[] = {
441441
BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, (BOOTPC << 16) + BOOTPS, 1, 0),
442442
BPF_STMT(BPF_RET + BPF_K, 0),
443443
};
444-
#define BPF_BOOTP_WRITE_LEN __arraycount(bpf_bootp_write)
444+
#define BPF_BOOTP_WRITE_LEN __arraycount(bpf_bootp_write)
445445

446446
#define BPF_BOOTP_CHADDR_LEN ((BOOTP_CHADDR_LEN / 4) * 3)
447447
#define BPF_BOOTP_XID_LEN 4 /* BOUND check is 4 instructions */
@@ -503,7 +503,7 @@ int
503503
bpf_filter_bootp(const struct bpf *bpf, __unused const struct in_addr *ia)
504504
{
505505
if (bpf_bootp_rw(bpf, true) == -1)
506-
return -1;
506+
return -1;
507507
if (bpf_bootp_rw(bpf, false) == -1 && errno != ENOSYS)
508508
return -1;
509509
if (bpf_lock(bpf) == -1 && errno != ENOSYS)

0 commit comments

Comments
 (0)