On Thu, 26 Sep 2019 at 10:59, Jason A. Donenfeld [off-list ref] wrote:

...

Instead what we’ve wound up with in this series is a Frankenstein’s
monster of Zinc, which appears to have basically the same goal as
Zinc, and even much of the same implementation just moved to a
different directory, but then skimps on making it actually work well
and introduces problems. (I’ll elucidate on some specific issues later
in this email so that we can get on the same page with regards to
security requirements for WireGuard.) I surmise from this Zinc-but-not
series is that what actually is going on here is mostly some kind of
power or leadership situation, which is what you’ve described to me
also at various other points and in person.

I'm not sure what you are alluding to here. I have always been very
clear about what I like about Zinc and what I don't like about Zinc.

I agree that it makes absolutely no sense for casual, in-kernel crypto
to jump through all the hoops that the crypto API requires. But for
operating on big chunks of data on the kernel heap, we have an
existing API that we should leverage if we can, and fix if we need to
so that all its users can benefit.

I also recognize that I am
at least part way to blame for whatever dynamic there has stagnated
this process; let me try to rectify that:

A principle objection you’ve had is that Zinc moves to its own
directory, with its own name, and tries to segment itself off from the
rest of the crypto API’s infrastructure. You’ve always felt this
should be mixed in with the rest of the crypto API’s infrastructure
and directory structures in one way or another. Let’s do both of those
things – put this in a directory structure you find appropriate and
hook this into the rest of the crypto API’s infrastructure in a way
you find appropriate. I might disagree, which is why Zinc does things
the way it does, but I’m open to compromise and doing things more your
way.

It doesn't have to be your way or my way. The whole point of being
part of this community is that we find solutions that work for
everyone, through discussion and iterative prototyping. Turning up out
of the blue with a 50,000 line patch set and a take-it-or-leave-it
attitude goes counter to that, and this is why we have made so little
progress over the past year.

But I am happy with your willingness to collaborate and find common
ground, which was also my motivation for spending a considerable
amount of time to prepare this patch set.

Another objection you’ve had is that Zinc replaces many existing
implementations with its own. Martin wasn’t happy about that either.
So let’s not do that, and we’ll have some wholesale replacement of
implementations in future patchsets at future dates discussed and
benched and bikeshedded independently from this.

Finally, perhaps most importantly, Zinc’s been my design rather than
our design. Let’s do this together instead of me git-send-email(1)-ing
a v37.

If the process of doing that together will be fraught with difficulty,
I’m still open to the “7 patch series” with the ugly cryptoapi.c
approach, as described at the top.

If your aim is to write ugly code and use that as a munition

But I think if we start with Zinc
and whittle it down in accordance with the above, we’ll get something
mutually acceptable, and somewhat similar to this series, with a few
important exceptions, which illustrate some of the issues I see in
this RFC:

Issue 1) No fast implementations for the “it’s just functions” interface.

This is a deal breaker. I know you disagree here and perhaps think all
dynamic dispatch should be by loadable modules configured with
userspace policy and lots of function pointers and dynamically
composable DSL strings, as the current crypto API does it. But I think
a lot of other people agree with me here (and they’ve chimed in
before) that the branch predictor does things better, doesn’t have
Spectre issues, and is very simple to read and understand. For
reference, here’s what that kind of thing looks like: [2].

This is one of the issues in the 'fix it for everyone else as well'
category. If we can improve the crypto API to be less susceptible to
these issues (e.g., using static calls), everybody benefits. I'd be
happy to collaborate on that.

In this case, the relevance is that the handshake in WireGuard is
extremely performance sensitive, in order to fend off DoS. One of the
big design gambits in WireGuard is – can we make it 1-RTT to reduce
the complexity of the state machine, but keep the crypto efficient
enough that this is still safe to do from a DoS perspective. The
protocol succeeds at this goal, but in many ways, just by a hair when
at scale, and so I’m really quite loathe to decrease handshake
performance.

...

Taken together, we simply can’t skimp on the implementations available
on the handshake layer, so we’ll need to add some form of
implementation selection, whether it’s the method Zinc uses ([2]), or
something else we cook up together.

So are you saying that the handshake timing constraints in the
WireGuard protocol are so stringent that we can't run it securely on,
e.g., an ARM CPU that lacks a NEON unit? Or given that you are not
providing accelerated implementations of blake2s or Curve25519 for
arm64, we can't run it securely on arm64 at all?

Typically, I would prefer to only introduce different versions of the
same algorithm if there is a clear performance benefit for an actual
use case.

Framing this as a security issue rather than a performance issue is
slightly disingenuous, since people are less likely to challenge it.
But the security of any VPN protocol worth its salt should not hinge
on the performance delta between the reference C code and a version
that was optimized for a particular CPU.

Issue 2) Linus’ objection to the async API invasion is more correct
than he realizes.

I could re-enumerate my objections to the API there, but I think we
all get it. It’s horrendous looking. Even the introduction of the
ivpad member (what on earth?) in the skb cb made me shutter.

Your implementation of RFC7539 truncates the nonce to 64-bits, while
RFC7539 defines a clear purpose for the bits you omit. Since the Zinc
library is intended to be standalone (and you are proposing its use in
other places, like big_keys.c), you might want to document your
justification for doing so in the general case, instead of ridiculing
the code I needed to write to work around this limitation.

But
there’s actually another issue at play:

wg_noise_handshake_begin_session→derive_keys→symmetric_key_init is all
part of the handshake. We cannot afford to allocate a brand new crypto
object, parse the DSL string, connect all those function pointers,
etc.

Parsing the string and connecting the function pointers happens only
once, and only when the transform needs to be instantiated from its
constituent parts. Subsequent invocations will just grab the existing
object.

The allocations involved here aren’t really okay at all in that
path. That’s why the cryptoapi.c idea above involves just using a pool
of pre-allocated objects if we’re going to be using that API at all.
Also keep in mind that WireGuard instances sometimes have hundreds of
thousands of peers.

My preference would be to address this by permitting per-request keys
in the AEAD layer. That way, we can instantiate the transform only
once, and just invoke it with the appropriate key on the hot path (and
avoid any per-keypair allocations)

I’d recommend leaving this synchronous as it exists now, as Linus
suggested, and we can revisit that later down the road. There are a
number of improvements to the async API we could make down the line
that could make this viable in WireGuard. For example, I could imagine
decoupling the creation of the cipher object from its keys and
intermediate buffers, so that we could in fact allocate the cipher
objects with their DSLs globally in a safe way, while allowing the
keys and working buffers to come from elsewhere. This is deep plumbing
into the async API, but I think we could get there in time.

My changes actually move all the rfc7539() intermediate buffers to the
stack, so the only remaining allocation is the per-keypair one.

There’s also a degree of practicality: right now there is zero ChaPoly
async acceleration hardware anywhere that would fit into the crypto
API. At some point, it might come to exist and have incredible
performance, and then we’ll both feel very motivated to make this work
for WireGuard. But it might also not come to be (AES seems to have won
over most of the industry), in which case, why hassle?

As I already pointed out, we have supported hardware already: CAAM is
in mainline, and Inside-Secure patches are on the list.

Issue 3) WireGuard patch is out of date.

This is my fault, because I haven’t posted in a long time. There are
some important changes in the main WireGuard repo. I’ll roll another
patch soon for this so we have something recent to work off of. Sorry
about that.

This is the reason I included your WG patch verbatim, to make it
easier to rebase to newer versions. In fact, I never intended or
expected anything but discussion from this submission, let alone
anyone actually merging it :-)

Issue 4) FPU register batching?

When I introduced the simd_get/simd_put/simd_relax thing, people
seemed to think it was a good idea. My benchmarks of it showed
significant throughput improvements. Your patchset doesn’t have
anything similar to this.

It uses the existing SIMD batching, and enhances it slightly for the
Poly1305/shash case.

But on the other hand, last I spoke with the
x86 FPU guys, I thought they might actually be in the process of
making simd_get/put obsolete with some internal plumbing to make
restoration lazier. I’ll see tglx later today and will poke him about
this, as this might already be a non-issue.

We've already made improvements here for arm64 as well (and ARM
already used lazy restore). But I think it still makes sense to
amortize these calls over a reasonable chunk of data, i.e., a packet.

So given the above, how would you like to proceed? My personal
preference would be to see you start with the Zinc patchset and rename
things and change the infrastructure to something that fits your
preferences, and we can see what that looks like. Less appealing would
be to do several iterations of you reworking Zinc from scratch and
going through the exercises all over again, but if you prefer that I
guess I could cope. Alternatively, maybe this is a lot to chew on, and
we should just throw caution into the wind, implement cryptoapi.c for
WireGuard (as described at the top), and add C functions to the crypto
API sometime later? This is what I had envisioned in [1].

It all depends on whether we are interested in supporting async
accelerators or not, and it is clear what my position is on this
point.

I am not convinced that we need accelerated implementations of blake2s
and curve25519, but if we do, I'd like those to be implemented as
individual modules under arch/*/crypto, with some moduleloader fu for
weak symbols or static calls thrown in if we have to. Exposing them as
shashes seems unnecessary to me at this point.

My only objection to your simd get/put interface is that it uses a
typedef rather than a struct definition (although I also wonder how we
can avoid two instances living on the same call stack, unless we
forbid functions that take a struct simd* to call functions that don't
take one, but these are details we should be able to work out.)

What I *don't* want is to merge WireGuard with its own library based
crypto now, and extend that later for async accelerators once people
realize that we really do need that as well.

And for the avoidance of doubt, or in case any of the above message
belied something different, I really am happy and relieved to have an
opportunity to work on this _with you_, and I am much more open than
before to compromise and finding practical solutions to the past
political issues. Also, if you’re into chat, we can always spec some
of the nitty-gritty aspects out over IRC or even the old-fashioned
telephone. Thanks again for pushing this forward.

My pleasure :-)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel