git: 74ae3f3e33b8 - main - if_wg: import latest fixup work from the wireguard-freebsd project

Kyle Evans kevans at freebsd.org
Mon Mar 15 15:27:56 UTC 2021


On Mon, Mar 15, 2021 at 9:57 AM Scott Long <scottl at samsco.org> wrote:
>
> Here is the response I sent to you and Donenfeld in private.  I won’t include
> my direct conversation with you from Slack/IRC, but I made my concerns
> and objections pretty clear.  This commit is quite disappointing.  See below:
>

I'm sorry that you feel that way, but I've been pretty vocal about my
intentions to merge this work as soon as we were done with internal
review due to the pre-existing state of the driver.

> The good:
> [... snip ...]
>
> The bad:
> - I want to be pragmatic about code APIs.  Maybe iflib isn’t ready for
> pseudo interfaces yet, and fixing it is non-trivial and out of scope.
> Going back to ifnet puts it back in line with openbsd and likely does
> fix the vnet problems.  However, it’s a radical change to the driver, and
> not something that I can support as a last-minute action for FreeBSD
> 13.0 or for pfSense.  Therefore, I’m advising against its immediate
> inclusion.  The final choice is not mine to make for FreeBSD, but
> that’s my recommendation.  For pfSense, I’ll be discussing this with
> the rest of the engineering staff on Monday.
>

iflib is definitely not ready for pseudo interfaces, and I'd like to
see the technical justification for using iflib here in the first
place. if_wg used exactly 0 of its real features because the queueing
system was bypassed by setting if_transmit in its IFDI_ATTACH_POST
implementation. In other words, we gained all of its complexity and
pseudo interfaces immaturity without any benefit that we found while
exploring the possibilities.

> - The LKML wouldn’t accept this kind of submission, they’d insist that
> it be broken down into consumable pieces, and that bug fixes be
> considered and provided that don’t rely on massive re-writes.  I’ve
> been dealing with linux for 20+ years and BSD for almost 30 years,
> and I’ve got to say that despite my distaste for how the LKML is run,
> they get results.  Does fixing a segfault or packet drop/reorder
> require the removal of iflib?
>

The review process on FreeBSD breaks down, as you yourself have noted.
mmacy has not been involved in if_wg since dropping it in the tree
AFAICT, and grehan claimed to only be involved because it was passed
to him at Netgate and that he didn't mind where it goes. Thus, I used
developer discretion and sought review from the person that wrote the
OpenBSD implementation and the person that designed the protocol. We
iterated on this for days to fix the numerous issues we were presented
with.

> The Ugly:
> - An accusation was made, tonight, to me, that the code Netgate
> sponsored was not reviewed and was shoved into the tree at the last
> minute.  This grossly ignores the actual history to the point of
> weakening my tolerance for this entire discussion.  It shows a pretty
> irrational bias against mmacy and Netgate and a gross ignorance of
> the history and provenience of the work.
>

I'm sorry that I got heated here, your tone was immediately aggressive
after I just spent five days cleaning up the mess that was left
behind. At least one of the security issues I found was a small
configuration tweak and near-immediate destruction of the system when
applying any real load to the driver.

> - The Netgate copyright was unilaterally removed.  Yes, it was re-
> instated a few minutes ago, and with good reason.  Please don’t
> do that again.
>

I won't touch on this, other than "ack".

> - The removal of the ASM crypto bits really confuses me.  An
> accusation was made, again tonight, that Netgate merely paid to
> benchmark the code, that we contributed nothing to it, and that it’s
> bad code that can’t be audited.  What I see is that the meat of the
> implementation is copyrighted by Jason and others.  There’s a
> modest but non-trivial amount of glue code that has (or had) the
> Netgate copyright. I’m not going to touch the audit nonsense.
> I need data here, and I’m not seeing it.
>

I would have appreciated a pointer to the copyrighted 63 lines in
include/, because this was obviously ignorance on my part. You
suggested functional testing and bug fixing, the former of which I
inherently include in 'benchmarking' since you can't benchmark
something that isn't functional, and received no pointer of the
latter.

> There seems to be a lot of bad blood, poor understanding, and
> misinformation going on.  I need all of this bullshit to stop.  This
> is 5000-ish lines of a nice way to get a point-to-point VPN going
> without the hassle of IPSec or the performance problems of
> OpenVPN.  It’s consuming way more time and energy than it’s
> worth to me, and I’d much rather spend time and money to
> implement OpenVPN DCO and work on profiling and optimizing
> IPSec.  Wireguard is important to Netgate because we recognize
> that it’s a feature that customers want, we’re committed to making
> security accessible, and we strongly believe in open source and
> FreeBSD. It’s not perfect code, not by a long shot, but I expect
> better collaboration and communication than what I’m seeing here.
>

It's important to me that we do what's right for the community, and
the if_wg module that was in-tree was not right for the community. I
just want something secure and usable in the tree, the latter point
being the packet loss complaints from the Netgate support forum that I
pointed you at as well as the kernel not handling allowed-ips
conflicts that I had mentioned, among various protocol violations and
other things the module did not handle w.r.t. configuration. The
former point being at least the buffer overflow I mentioned, but there
are more.

Thanks,

Kyle Evans

> Scott
>
>
> > On Mar 14, 2021, at 10:52 PM, Kyle Evans <kevans at FreeBSD.org> wrote:
> >
> > The branch main has been updated by kevans:
> >
> > URL: https://cgit.FreeBSD.org/src/commit/?id=74ae3f3e33b810248da19004c58b3581cd367843
> >
> > commit 74ae3f3e33b810248da19004c58b3581cd367843
> > Author:     Kyle Evans <kevans at FreeBSD.org>
> > AuthorDate: 2021-03-15 02:25:40 +0000
> > Commit:     Kyle Evans <kevans at FreeBSD.org>
> > CommitDate: 2021-03-15 04:52:04 +0000
> >
> >    if_wg: import latest fixup work from the wireguard-freebsd project
> >
> >    This is the culmination of about a week of work from three developers to
> >    fix a number of functional and security issues.  This patch consists of
> >    work done by the following folks:
> >
> >    - Jason A. Donenfeld <Jason at zx2c4.com>
> >    - Matt Dunwoodie <ncon at noconroy.net>
> >    - Kyle Evans <kevans at FreeBSD.org>
> >
> >    Notable changes include:
> >    - Packets are now correctly staged for processing once the handshake has
> >      completed, resulting in less packet loss in the interim.
> >    - Various race conditions have been resolved, particularly w.r.t. socket
> >      and packet lifetime (panics)
> >    - Various tests have been added to assure correct functionality and
> >      tooling conformance
> >    - Many security issues have been addressed
> >    - if_wg now maintains jail-friendly semantics: sockets are created in
> >      the interface's home vnet so that it can act as the sole network
> >      connection for a jail
> >    - if_wg no longer fails to remove peer allowed-ips of 0.0.0.0/0
> >    - if_wg now exports via ioctl a format that is future proof and
> >      complete.  It is additionally supported by the upstream
> >      wireguard-tools (which we plan to merge in to base soon)
> >    - if_wg now conforms to the WireGuard protocol and is more closely
> >      aligned with security auditing guidelines
> >
> >    Note that the driver has been rebased away from using iflib.  iflib
> >    poses a number of challenges for a cloned device trying to operate in a
> >    vnet that are non-trivial to solve and adds complexity to the
> >    implementation for little gain.
> >
> >    The crypto implementation that was previously added to the tree was a
> >    super complex integration of what previously appeared in an old out of
> >    tree Linux module, which has been reduced to crypto.c containing simple
> >    boring reference implementations.  This is part of a near-to-mid term
> >    goal to work with FreeBSD kernel crypto folks and take advantage of or
> >    improve accelerated crypto already offered elsewhere.
> >
> >    There's additional test suite effort underway out-of-tree taking
> >    advantage of the aforementioned jail-friendly semantics to test a number
> >    of real-world topologies, based on netns.sh.
> >
> >    Also note that this is still a work in progress; work going further will
> >    be much smaller in nature


More information about the dev-commits-src-all mailing list