WireGuard bounces off FreeBSD—for now

> That doesn't look as terrible as Donenfeld implies, but his points seem basically true,

There are more bugs than the ones you identified. Copying and pasting from https://cgit.freebsd.org/src/commit/?id=2338da0373f19b511... , for example:

+ out_len = sizeof(struct wg_pkt_data) + plaintext_len + NOISE_MAC_SIZE;
+
+ if ((mc = m_getjcl(M_NOWAIT, MT_DATA, M_PKTHDR, MCLBYTES)) == NULL)
+ goto error;
+
+ data = mtod(mc, struct wg_pkt_data *);
+ m_copydata(m, 0, m->m_pkthdr.len, data->data.buf);

If out_len is greater than out MCLBYTES (2048), that m_copydata overflows a heap buffer.

+ /* A packet with length 0 is a keepalive packet */
+ if (m->m_pkthdr.len == 0) {
+ DPRINTF(peer->p_sc, "Receiving keepalive packet from peer "
+ "%llu\n", (unsigned long long)peer->p_id);
+ goto done;
+ }
+
+ version = mtod(m, struct ip *)->ip_v;
+ if (version != IPVERSION && version != 6) {
+ DPRINTF(peer->p_sc, "Packet is neither ipv4 nor ipv6 from peer "
+ "%llu\n", (unsigned long long)peer->p_id);
+ goto error;
+ }
+
+ routed_peer = wg_route_lookup(&peer->p_sc->sc_routes, m, IN);

It checks for len == 0, but doesn't check for len >= 20 or >= 40, for being able to read the IPv4/IPv6 headers, so you wind overrunning and reading nonsense off the heap. Later this too-short packet is passed onto directly to the rest of the networking stack.

+ aip = malloc(count*sizeof(*aip), M_TEMP, M_WAITOK);

Probably should be using `mallocarray` on that multiplication.

+ if ((m = m_defrag(m0, M_NOWAIT)) == NULL) {
+ DPRINTF(sc, "DEFRAG fail\n");
+ return;
+ }
+ data = mtod(m, void *);

m_defrag organizes the chain into optimally sized fragments. If the data doesn't fit into a single fragment, it winds up just being truncated and then later decrypted?

+void curve25519_generic(u8 out[CURVE25519_KEY_SIZE],
+ const u8 scalar[CURVE25519_KEY_SIZE],
+ const u8 point[CURVE25519_KEY_SIZE])
+{
+ fe x1, x2, z2, x3, z3;
+ fe_loose x2l, z2l, x3l;
+ unsigned swap = 0;
+ int pos;
+ u8 e[32];
+
+ memcpy(e, scalar, 32);
+
+ /* The following implementation was

What happened to the call to clamp that scalar? DJB's paper pretty clearly lays out the case for that.

+ if (curthread->td_ucred->cr_uid == 0)
+ nvlist_add_binary(nvl, "private-key", sc->sc_local.l_private, WG_KEY_SIZE);

Just like on Linux, where checking current->uid is not the right way and you should use capable or ns_capable, so too here priv_check should be used.

...and so on and so forth.

It really was as bad as I described.

Anyone, mainstream kernel developer or writer of some kernel add-on, that writes kernel level code, should be striving to be as close to perfect, or at least perfectly reliable and decently performant, as can be achieved.

MAYBE SeL4 gets close.

In the long run, only AI's should code; humans are too defective; whether too sloppy or too egotistical matters not.