diff mbox series

[net-next,v3,02/17] zinc: introduce minimal cryptography library

Message ID 20180911010838.8818-3-Jason@zx2c4.com
State New
Headers show
Series WireGuard: Secure Network Tunnel | expand

Commit Message

Jason A. Donenfeld Sept. 11, 2018, 1:08 a.m. UTC
Zinc stands for "Zinc Is Neat Crypto" or "Zinc as IN Crypto" or maybe
just "Zx2c4's INsane Cryptolib." It's also short, easy to type, and
plays nicely with the recent trend of naming crypto libraries after
elements. The guiding principle is "don't overdo it". It's less of a
library and more of a directory tree for organizing well-curated direct
implementations of cryptography primitives.

Zinc is a new cryptography API that is much more minimal and lower-level
than the current one. It intends to complement it and provide a basis
upon which the current crypto API might build, as the provider of
software implementations of cryptographic primitives. It is motivated by
three primary observations in crypto API design:

  * Highly composable "cipher modes" and related abstractions from
    90s cryptographers did not turn out to be as terrific an idea as
    hoped, leading to a host of API misuse problems.

  * Most programmers are afraid of crypto code, and so prefer to
    integrate it into libraries in a highly abstracted manner, so as to
    shield themselves from implementation details. Cryptographers, on
    the other hand, prefer simple direct implementations, which they're
    able to verify for high assurance and optimize in accordance with
    their expertise.

  * Overly abstracted and flexible cryptography APIs lead to a host of
    dangerous problems and performance issues. The kernel is in the
    business usually not of coming up with new uses of crypto, but
    rather implementing various constructions, which means it essentially
    needs a library of primitives, not a highly abstracted enterprise-ready
    pluggable system, with a few particular exceptions.

This last observation has seen itself play out several times over and
over again within the kernel:

  * The perennial move of actual primitives away from crypto/ and into
    lib/, so that users can actually call these functions directly with
    no overhead and without lots of allocations, function pointers,
    string specifier parsing, and general clunkiness. For example:
    sha256, chacha20, siphash, sha1, and so forth live in lib/ rather
    than in crypto/. Zinc intends to stop the cluttering of lib/ and
    introduce these direct primitives into their proper place, lib/zinc/.

  * An abundance of misuse bugs with the present crypto API that have
    been very unpleasant to clean up.

  * A hesitance to even use cryptography, because of the overhead and
    headaches involved in accessing the routines.

Zinc goes in a rather different direction. Rather than providing a
thoroughly designed and abstracted API, Zinc gives you simple functions,
which implement some primitive, or some particular and specific
construction of primitives. It is not dynamic in the least, though one
could imagine implementing a complex dynamic dispatch mechanism (such as
the current crypto API) on top of these basic functions. After all,
dynamic dispatch is usually needed for applications with cipher agility,
such as IPsec, dm-crypt, AF_ALG, and so forth, and the existing crypto
API will continue to play that role. However, Zinc will provide a non-
haphazard way of directly utilizing crypto routines in applications
that do have neither the need nor desire for abstraction and dynamic
dispatch.

It also organizes the implementations in a simple, straight-forward,
and direct manner, making it enjoyable and intuitive to work on.
Rather than moving optimized assembly implementations into arch/, it
keeps them all together in lib/zinc/, making it simple and obvious to
compare and contrast what's happening. This is, notably, exactly what
the lib/raid6/ tree does, and that seems to work out rather well. It's
also the pattern of most successful crypto libraries. The architecture-
specific glue-code is made a part of each translation unit, rather than
being in a separate one, so that generic and architecture-optimized code
are combined at compile-time, and incompatibility branches compiled out by
the optimizer.

All implementations have been extensively tested and fuzzed, and are
selected for their quality, trustworthiness, and performance. Wherever
possible and performant, formally verified implementations are used,
such as those from HACL* [1] and Fiat-Crypto [2]. The routines also take
special care to zero out secrets using memzero_explicit (and future work
is planned to have gcc do this more reliably and performantly with
compiler plugins). The performance of the selected implementations is
state-of-the-art and unrivaled on a broad array of hardware, though of
course we will continue to fine tune these to the hardware demands
needed by kernel contributors. Each implementation also comes with
extensive self-tests and crafted test vectors, pulled from various
places such as Wycheproof [9].

Regularity of function signatures is important, so that users can easily
"guess" the name of the function they want. Though, individual
primitives are oftentimes not trivially interchangeable, having been
designed for different things and requiring different parameters and
semantics, and so the function signatures they provide will directly
reflect the realities of the primitives' usages, rather than hiding it
behind (inevitably leaky) abstractions. Also, in contrast to the current
crypto API, Zinc functions can work on stack buffers, and can be called
with different keys, without requiring allocations or locking.

SIMD is used automatically when available, though some routines may
benefit from either having their SIMD disabled for particular
invocations, or to have the SIMD initialization calls amortized over
several invocations of the function, and so Zinc utilizes function
signatures enabling that in conjunction with the recently introduced
simd_context_t.

More generally, Zinc provides function signatures that allow just what
is required by the various callers. This isn't to say that users of the
functions will be permitted to pollute the function semantics with weird
particular needs, but we are trying very hard not to overdo it, and that
means looking carefully at what's actually necessary, and doing just that,
and not much more than that. Remember: practicality and cleanliness rather
than over-zealous infrastructure.

Zinc provides also an opening for the best implementers in academia to
contribute their time and effort to the kernel, by being sufficiently
simple and inviting. In discussing this commit with some of the best and
brightest over the last few years, there are many who are eager to
devote rare talent and energy to this effort.

Following the merging of this, I expect for the primitives that
currently exist in lib/ to work their way into lib/zinc/, after intense
scrutiny of each implementation, potentially replacing them with either
formally-verified implementations, or better studied and faster
state-of-the-art implementations.

Also following the merging of this, I expect for the old crypto API
implementations to be ported over to use Zinc for their software-based
implementations.

As Zinc is simply library code, its config options are un-menued, with
the exception of CONFIG_ZINC_DEBUG, which enables various selftests and
BUG_ONs.

[1] https://github.com/project-everest/hacl-star
[2] https://github.com/mit-plv/fiat-crypto
[3] https://cr.yp.to/ecdh.html
[4] https://cr.yp.to/chacha.html
[5] https://cr.yp.to/snuffle/xsalsa-20081128.pdf
[6] https://cr.yp.to/mac.html
[7] https://blake2.net/
[8] https://tools.ietf.org/html/rfc8439
[9] https://github.com/google/wycheproof

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: Samuel Neves <sneves@dei.uc.pt>
Cc: Jean-Philippe Aumasson <jeanphilippe.aumasson@gmail.com>
Cc: linux-crypto@vger.kernel.org
---
 MAINTAINERS       |  8 ++++++++
 lib/Kconfig       |  2 ++
 lib/Makefile      |  2 ++
 lib/zinc/Kconfig  | 20 ++++++++++++++++++++
 lib/zinc/Makefile |  8 ++++++++
 lib/zinc/main.c   | 31 +++++++++++++++++++++++++++++++
 6 files changed, 71 insertions(+)
 create mode 100644 lib/zinc/Kconfig
 create mode 100644 lib/zinc/Makefile
 create mode 100644 lib/zinc/main.c

-- 
2.18.0

Comments

Andy Lutomirski Sept. 11, 2018, 10:16 p.m. UTC | #1
> On Sep 11, 2018, at 2:47 PM, Eric Biggers <ebiggers@kernel.org> wrote:

> 

>> On Tue, Sep 11, 2018 at 04:56:24PM +0200, Greg Kroah-Hartman wrote:

>> On Tue, Sep 11, 2018 at 12:08:56PM +0200, Ard Biesheuvel wrote:

>>>> As Zinc is simply library code, its config options are un-menued, with

>>>> the exception of CONFIG_ZINC_DEBUG, which enables various selftests and

>>>> BUG_ONs.

>>>> 

>>> 

>>> In spite of the wall of text, you fail to point out exactly why the

>>> existing AEAD API in unsuitable, and why fixing it is not an option.

>>> 

>>> As I pointed out in a previous version, I don't think we need a

>>> separate crypto API/library in the kernel, and I don't think you have

>>> convinced anyone else yet either.

>> 

>> Um, then why do people keep sprinkling new crypto/hash code all around

>> the kernel tree?  It's because what we have as a crypto api is complex

>> and is hard to use for many in-kernel users.

>> 

>> Something like this new interface (zinc) is a much "saner" api for

>> writing kernel code that has to interact with crypto/hash primitives.

>> 

>> I see no reason why the existing crypto code can be redone to use the

>> zinc crypto primitives over time, making there only be one main location

>> for the crypto algorithms.  But to do it the other way around is pretty

>> much impossible given the complexities in the existing api that has been

>> created over time.

>> 

>> Not to say that the existing api is not a viable one, but ugh, really?

>> You have to admit it is a pain to try to use in any "normal" type of

>> "here's a bytestream, go give me a hash" type of method, right?

>> 

>> Also there is the added benefit that the crypto primitives here have

>> been audited by a number of people (so Jason stated), and they are

>> written in a way that the crypto community can more easily interact and

>> contribute to.  Which is _way_ better than what we have today.

>> 

>> So this gets my "stamp of approval" for whatever it is worth :)

>> 

> 

> I think you mean you see no reason why it *cannot* be converted?  The

> conversions definitely *should* be done, just like how some of the existing

> crypto API algorithms like SHA-256 already wrap implementations in lib/.  In my

> view, lib/zinc/ isn't fundamentally different from what we already have for some

> algorithms.  So it's misguided to design/present it as some novel thing, which

> unfortunately this patchset still does to a large extent.  (The actual new thing

> is how architecture-specific implementations are handled.)

> 

> Of course, the real problem is that even after multiple revisions of this

> patchset, there's still no actual conversions of the existing crypto API

> algorithms over to use the new implementations.  "Zinc" is still completely

> separate from the existing crypto API.

> 


Jason, can you do one of these conversions as an example?

> So, it's not yet clear that the conversions will actually work out without

> problems that would require changes in "Zinc".  I don't think it makes sense to

> merge all this stuff without doing the conversions, or at the very least

> demonstrating how they will be done.

> 

> In particular, in its current form "Zinc" is useless for anyone that needs the

> existing crypto API.  For example, for HPolyC,

> (https://lkml.org/lkml/2018/8/6/857), I need to make improvements to ChaCha and

> Poly1305 in the existing crypto API, e.g. to add support for XChaCha and

> NEON-accelerated Poly1305.  Having completely separate ChaCha and Poly1305

> implementations in Zinc doesn't help at all.  If anything, it makes things

> harder because people will have to review/maintain both sets of implementations;

> and when trying to make the improvements I need, I'll find myself in the middle

> of a holy war between two competing camps who each have their own opinion about

> The Right Way To Do Crypto, and their own crypto implementations and APIs in the

> kernel.

> 

> - Eric
Andy Lutomirski Sept. 11, 2018, 11:01 p.m. UTC | #2
> On Sep 11, 2018, at 3:18 PM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:

> 

>> On Tue, Sep 11, 2018 at 4:16 PM Andy Lutomirski <luto@amacapital.net> wrote:

>> Jason, can you do one of these conversions as an example?

> 

> My preference is really to leave that to a different patch series. But

> if you think I *must*, then I shall.

> 

> 


I think Ard’s point is valid: in the long run we don’t want two competing software implementations of each primitive. It clearly *should* be possible to make crypto API call into zinc for synchronous software operations, but a demonstration of how this actually works and that there isn’t some change to zinc to make it would well would be in order, I think.

IMO the right approach is do one conversion right away and save the rest for later.
Eric Biggers Sept. 12, 2018, 6:34 p.m. UTC | #3
On Wed, Sep 12, 2018 at 08:19:21PM +0200, Ard Biesheuvel wrote:
> On 12 September 2018 at 20:16, Jason A. Donenfeld <Jason@zx2c4.com> wrote:

> > Hi Eric,

> >

> > On Wed, Sep 12, 2018 at 12:08 AM Eric Biggers <ebiggers@kernel.org> wrote:

> >> I'd strongly prefer the assembly to be readable too.  Jason, I'm not sure if

> >> you've actually read through the asm from the OpenSSL implementations, but the

> >> generated .S files actually do lose a lot of semantic information that was in

> >> the original .pl scripts.

> >

> > The thing to keep in mind is that the .S was not directly and blindly

> > generated from the .pl. We started with the output of the .pl, and

> > then, particularly in the case of x86_64, worked with it a lot, and

> > now it's something a bit different. We've definitely spent a lot of

> > time reading that assembly.

> >

> 

> Can we please have those changes as a separate patch? Preferably to

> the .pl file rather than the .S file, so we can easily distinguish the

> code from upstream from the code that you modified.

> 

> > I'll see if I can improve the readability with some register name

> > remapping on ARM. No guarantees, but I'll play a bit and see if I can

> > make it a bit better.

> >

> > Jason


FWIW, yesterday I made a modified version of poly1305-armv4.pl that generates an
asm file that works in kernel mode.  The changes are actually pretty small, and
I think we can get them upstream into OpenSSL like they were for sha256-armv4.pl
and sha512-armv4.pl.  I'll start a thread with Andy Polyakov and you two.

But I don't have time to help with all the many OpenSSL asm files Jason is
proposing, just maybe poly1305-armv4 and chacha-armv4 for now.

- Eric
Ard Biesheuvel Sept. 13, 2018, 5:41 a.m. UTC | #4
On 13 September 2018 at 01:45, Andy Lutomirski <luto@kernel.org> wrote:
> On Wed, Sep 12, 2018 at 3:56 PM, Ard Biesheuvel

> <ard.biesheuvel@linaro.org> wrote:

>> I realize you've put a lot of good and hard work into the existing

>> I am also concerned about your claim that all software algorithms will

>> be moved into this crypto library. You are not specific about whose

>> responsibility it will be that this is going to happen in a timely

>> fashion. But more importantly, it is not clear at all how you expect

>> this to work for, e.g., h/w instruction based SHAxxx or AES in various

>> chaining modes, which should be used only on cores that implement

>> those instructions (note that on arm64, we have optional instructions

>> for AES, PMULL, SHA1, SHA256, SHA512, SHA3, SM3 and SM4). Are all

>> those implementations (only few of which will be used on a certain

>> core) going to be part of the monolithic library? What are the APIs

>> going to look like for block ciphers, taking chaining modes into

>> account?

>

> I'm not convinced that there's any real need for *all* crypto

> algorithms to move into lib/zinc or to move at all.  As I see it,

> there are two classes of crypto algorithms in the kernel:

>

> a) Crypto that is used by code that chooses its algorithm statically

> and wants synchronous operations.  These include everything in

> drivers/char/random.c, but also a bunch of various networking things

> that are hardcoded and basically everything that uses stack buffers.

> (This means it includes all the code that I broke when I did

> VMAP_STACK.  Sign.)

>

> b) Crypto that is used dynamically.  This includes dm-crypt

> (aes-xts-plain64, aes-cbc-essiv, etc), all the ALG_IF interfaces, a

> lot of IPSEC stuff, possibly KCM, and probably many more.  These will

> get comparatively little benefit from being converted to a zinc-like

> interface.  For some of these cases, it wouldn't make any sense at all

> to convert them.  Certainly the ones that do async hardware crypto

> using DMA engines will never look at all like zinc, even under the

> hood.

>

> I think that, as a short-term goal, it makes a lot of sense to have

> implementations of the crypto that *new* kernel code (like Wireguard)

> wants to use in style (a) that live in /lib, and it obviously makes

> sense to consolidate their implementations with the crypto/

> implementations in a timely manner.  As a medium-term goal, adding

> more algorithms as needed for things that could use the simpler APIs

> (Bluetooth, perhaps) would make sense.

>

> But I see no reason at all that /lib should ever contain a grab-bag of

> crypto implementations just for the heck of it.  They should have real

> in-kernel users IMO.  And this means that there will probably always

> be some crypto implementations in crypto/ for things like aes-xts.

>


But one of the supposed selling points of this crypto library is that
it gives engineers who are frightened of crypto in general and the
crypto API in particular simple and easy to use crypto primitives
rather than having to jump through the crypto API's hoops.

A crypto library whose only encryption algorithm is a stream cipher
does *not* deliver on that promise, since it is only suitable for
cases where IVs are guaranteed not to be reused. You yourself were
bitten by the clunkiness of the crypto API when attempting to use the
SHA26 code, right? So shouldn't we move that into this crypto library
as well?

I think it is reasonable for WireGuard to standardize on
ChaCha20/Poly1305 only, although I have my concerns about the flag day
that will be required if this 'one true cipher' ever does turn out to
be compromised (either that, or we will have to go back in time and
add some kind of protocol versioning to existing deployments of
WireGuard)

And frankly, if the code were as good as the prose, we wouldn't be
having this discussion. Zinc adds its own clunky ways to mix arch and
generic code, involving GCC -include command line arguments and
#ifdefs everywhere. My review comments on this were completely ignored
by Jason.
Jason A. Donenfeld Sept. 13, 2018, 2:15 p.m. UTC | #5
Hi Ard,

On Thu, Sep 13, 2018 at 12:56 AM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> In this series, you are dumping a huge volume of unannotated,

> generated asm into the kernel which has been modified [by you] to

> [among other things?] adhere to the kernel API (without documenting

> what the changes are exactly). How does that live up to the promise of

> better, peer reviewed code?


The code still benefits from the review that's gone into OpenSSL. It's
not modified in ways that would affect the cryptographic operations
being done. It's modified to be suitable for kernel space.

> Then there is the performance claim. We know for instance that the

> OpenSSL ARM NEON code for ChaCha20 is faster on cores that happen to

> possess a micro-architectural property that ALU instructions are

> essentially free when they are interleaved with SIMD instructions. But

> we also know that a) Cortex-A7, which is a relevant target, is not one

> of those cores, and b) that chip designers are not likely to optimize

> for that particular usage pattern so relying on it in generic code is

> unwise in general.


That's interesting. I'll bring this up with AndyP. FWIW, if you think
you have a real and compelling claim here, I'd be much more likely to
accept a different ChaCha20 implementation than I would be to accept a
different Poly1305 implementation. (It's a *lot* harder to screw up
ChaCha20 than it is to screw up Poly1305.)

> I am also concerned about your claim that all software algorithms will

> be moved into this crypto library.


I'll defer to Andy's response here, which I think is a correct one:
https://lkml.org/lkml/2018/9/13/27

The short answer is that Zinc is going to be adding the ciphers that
people want to use for normal reasons from normal code. For example,
after this merges, we'll next be working on moving the remaining
non-optimized C code out of lib/ that's called by places (such as
SHA2).

> You are not specific about whose

> responsibility it will be that this is going to happen in a timely

> fashion.


I thought I laid out the roadmap for this in the commit message. In
case I wasn't clear: my plan is to tackle lib/ after merging, and I
plan to do so in a timely manner. It's a pretty common tactic to keep
layering on tasks, "what about X?", "what about Y?", "I won't agree
unless Z!" -- when in reality kernel development and refactorings are
done incrementally. I've been around on this list contributing code
for long enough that you should have a decent amount of confidence
that I'm not just going to disappear working on this or something
insane like that. And neither are the two academic cryptographers CC'd
on this thread. So, as Andy said, we're going to be porting to Zinc
the primitives that are useful for the various applications of Zinc.
This means yes, we'll have SHA2 in there.

> chaining modes

> What are the APIs

> going to look like for block ciphers, taking chaining modes into

> account?


As mentioned in the commit message and numerous times, we're not
trying to make a win32-like crypto API here or to remake the existing
Linux crypto API. Rather we're providing libraries of specific
functions that are useful for various circumstances. For example, if
AES-GCM is desired at some point, then we'll have a similar API for
that as we do for ChaPoly -- one that takes buffers and one that takes
sg. Likewise, hash functions use the familiar init/update/final.
"Generic" chaining modes aren't really part of the equation or design
goals.

Again, I realize you've spent a long time working on the existing
crypto API, and so your questions and concerns are in the line of,
"how are we going to make Zinc look like the existing crypto API in
functionality?" But that's not what we're up to here. We have a
different and complementary design goal. I understand why you're
squirming, but please recognize we're working on different things.

> I'm sure it is rather simple to port the crypto API implementation of

> ChaCha20 to use your library. I am more concerned about how your

> library is going to expand to cover all other software algorithms that

> we currently use in the kernel.


The subset of algorithms we add will be developed with the same
methodology as the present ones. There is nothing making this
particularly difficult or even more difficult for other primitives
than it was for ChaCha20. It's especially easy, in fact, since we're
following similar design methodologies as the vast majority of other
cryptography libraries that have been developed. Namely, we're
creating simple things called "functions".

> Of course. But please respond to all the concerns,

> You have not

> responded to that concern yet.


Sorry, it's certainly not my intention. I've been on vacation with my
family for the last several weeks, and only returned home
sleep-deprived last night after 4 days of plane delays. I've now
rested and will resume working on this full-time and I'll try my best
to address concerns, and also go back through emails to find things I
might have missed. (First, though, I'm going to deal with getting back
the three suitcases the airline lost in transit...)

> > Anyway, it sounds like this whole thing may have ruffled your feathers

> > a bit. Will you be at Linux Plumbers Conference in November? I'm

> > planning on attending, and perhaps we could find some time there to

> > sit down and talk one on one a bit.

>

> That would be good, yes. I will be there.


Looking forward to talking to you there, and hopefully we can put to
rest any lingering concerns.

Jason
Jason A. Donenfeld Sept. 13, 2018, 2:18 p.m. UTC | #6
On Thu, Sep 13, 2018 at 1:45 AM Andy Lutomirski <luto@kernel.org> wrote:
> I'm not convinced that there's any real need for *all* crypto

> algorithms to move into lib/zinc or to move at all.  As I see it,

> there are two classes of crypto algorithms in the kernel:

>

> a) Crypto that is used by code that chooses its algorithm statically

> and wants synchronous operations.  These include everything in

> drivers/char/random.c, but also a bunch of various networking things

> that are hardcoded and basically everything that uses stack buffers.

> (This means it includes all the code that I broke when I did

> VMAP_STACK.  Sign.)


Right, exactly. This is what will wind up using Zinc. I'm working on
an example usage of this for v4 of the patch submission, which you can
ogle in a preview here if you're curious:

https://git.zx2c4.com/linux-dev/commit/?h=big_key_rewrite

28 insertions, 206 deletions :-D

> b) Crypto that is used dynamically.  This includes dm-crypt

> (aes-xts-plain64, aes-cbc-essiv, etc), all the ALG_IF interfaces, a

> lot of IPSEC stuff, possibly KCM, and probably many more.  These will

> get comparatively little benefit from being converted to a zinc-like

> interface.  For some of these cases, it wouldn't make any sense at all

> to convert them.  Certainly the ones that do async hardware crypto

> using DMA engines will never look at all like zinc, even under the

> hood.


Right, this is what the crypto API will continue to be used for.


> I think that, as a short-term goal, it makes a lot of sense to have

> implementations of the crypto that *new* kernel code (like Wireguard)

> wants to use in style (a) that live in /lib, and it obviously makes

> sense to consolidate their implementations with the crypto/

> implementations in a timely manner.  As a medium-term goal, adding

> more algorithms as needed for things that could use the simpler APIs

> (Bluetooth, perhaps) would make sense.


Agreed 100%. With regards to "consolidate their implementations" --
I've actually already done this after your urging yesterday, and so
that will be a part of v4.

> But I see no reason at all that /lib should ever contain a grab-bag of

> crypto implementations just for the heck of it.  They should have real

> in-kernel users IMO.  And this means that there will probably always

> be some crypto implementations in crypto/ for things like aes-xts.


Right, precisely.

Jason
Andy Lutomirski Sept. 13, 2018, 3:26 p.m. UTC | #7
> On Sep 12, 2018, at 11:39 PM, Milan Broz <gmazyland@gmail.com> wrote:

> 

>> On 13/09/18 01:45, Andy Lutomirski wrote:

>> On Wed, Sep 12, 2018 at 3:56 PM, Ard Biesheuvel

> ... 

>> b) Crypto that is used dynamically.  This includes dm-crypt

>> (aes-xts-plain64, aes-cbc-essiv, etc), all the ALG_IF interfaces, a

>> lot of IPSEC stuff, possibly KCM, and probably many more.  These will

>> get comparatively little benefit from being converted to a zinc-like

>> interface.  For some of these cases, it wouldn't make any sense at all

>> to convert them.  Certainly the ones that do async hardware crypto

>> using DMA engines will never look at all like zinc, even under the

>> hood.

> 

> Please note, that dm-crypt now uses not only block ciphers and modes,

> but also authenticated encryption and hashes (for ESSIV and HMAC

> in authenticated composed modes) and RNG (for random IV).

> We use crypto API, including async variants (I hope correctly :)


Right. And all this is why I don’t think dm-crypt should use zinc, at least not any time soon.
Ard Biesheuvel Sept. 13, 2018, 3:42 p.m. UTC | #8
On 13 September 2018 at 16:32, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> On Thu, Sep 13, 2018 at 7:41 AM Ard Biesheuvel

> <ard.biesheuvel@linaro.org> wrote:

>> But one of the supposed selling points of this crypto library is that

>> it gives engineers who are frightened of crypto in general and the

>> crypto API in particular simple and easy to use crypto primitives

>> rather than having to jump through the crypto API's hoops.

>

> The goal is for engineers who want to specifically use algorithm X

> from within the kernel in a non-dynamic way to be able to then use

> algorithm X with a simple function call. The goal is not to open it up

> to people who have no idea what they're doing; for that a NaCL-like

> library with functions like "crypto_box_open" or something would fit

> the bill; but that's also not what we're trying to do here. Please

> don't confuse the design goals. The rest of your email is therefore a

> bit of a straw man; cut the rhetoric out.

>

>> A crypto library whose only encryption algorithm is a stream cipher

>> does *not* deliver on that promise, since it is only suitable for

>> cases where IVs are guaranteed not to be reused.

>

> False. We also offer XChaCha20Poly1305, which takes a massive nonce,

> suitable for random generation.

>

> If there became a useful case for AES-PMAC-SIV or even AES-GCM or

> something to that extent, then Zinc would add that as required. But

> we're not going to start adding random ciphers unless they're needed.

>


I'd prefer it if all the accelerated software implementations live in
the same place. But I do strongly prefer arch code to live in
arch/$arch, simply because of the maintenance scope for arch
developers and maintainers.

I think AES-GCM is a useful example here. I really like the SIMD token
abstraction a lot, but I would like to understand how this would work
in Zinc if you have
a) a generic implementation
b) perhaps an arch specific scalar implementation
c) a pure NEON implementation
d) an implementation using AES instructions but not the PMULL instructions
e) an implementation that uses AES and PMULL instructions.

On arch/arm64 we support code patching, on other arches it may be
different. We also support udev loading of modules depending on CPU
capabilities, which means only the implementation you are likely to
use will be loaded, and other modules are disregarded.

I am not asking you to implement this now. But I do want to understand
how these use cases fit into Zinc. And note that this has nothing to
do with the crypto API: this could simply be a synchronous
aes_gcm_encrypt() library function that maps to any of the above at
runtime depending on the platform's capabilities.

>> You yourself were

>> bitten by the clunkiness of the crypto API when attempting to use the

>> SHA26 code, right? So shouldn't we move that into this crypto library

>> as well?

>

> As stated in the initial commit, and in numerous other emails

> stretching back a year, yes, sha256 and other things in lib/ are going

> to be put into Zinc following the initial merge of Zinc. These changes

> will happen incrementally, like everything else that happens in the

> kernel. Sha256, in particular, is probably the first thing I'll port

> post-merge.

>


Excellent.

>> I think it is reasonable for WireGuard to standardize on

>> ChaCha20/Poly1305 only, although I have my concerns about the flag day

>> that will be required if this 'one true cipher' ever does turn out to

>> be compromised (either that, or we will have to go back in time and

>> add some kind of protocol versioning to existing deployments of

>> WireGuard)

>

> Those concerns are not valid and have already been addressed (to you,

> I believe) on this mailing list and elsewhere. WireGuard is versioned,

> hence there's no need to "add" versioning, and it is prepared to roll

> out new cryptography in a subsequent version should there be any

> issues. In other words, your concern is based on a misunderstanding of

> the protocol. If you have issues, however, with the design decisions

> of WireGuard, something that's been heavily discussed with members of

> the linux kernel community, networking community, cryptography

> community, and so forth, for the last 3 years, I invite you to bring

> them up on <wireguard@lists.zx2c4.com>.

>


I'd prefer to have the discussion here, if you don't mind.

IIUC clients and servers need to run the same version of the protocol.
So does it require a flag day to switch the world over to the new
version when the old one is deprecated?

>> And frankly, if the code were as good as the prose, we wouldn't be

>> having this discussion.

>

> Please cut out this rhetoric. That's an obviously unprovable

> statement, but it probably isn't true anyway. I wish you'd stick to

> technical concerns only, rather than what appears to be a desire to

> derail this by any means necessary.

>


I apologize if I hit a nerve there, that was not my intention.

I am not an 'entrenched crypto API guy that is out to get you'. I am a
core ARM developer that takes an interest in crypto, shares your
concern about the usability of the crypto API, and tries to ensure
that what we end up is maintainable and usable for everybody.

>> Zinc adds its own clunky ways to mix arch and

>> generic code, involving GCC -include command line arguments and

>> #ifdefs everywhere. My review comments on this were completely ignored

>> by Jason.

>

> No, they were not ignored. v2 cleaned up the #ifdefs. v4 has already

> cleaned up the makefile stuff and will be even cleaner. Good things

> await, don't worry.

>


You know what? If you're up for it, let's not wait until Plumbers, but
instead, let's collaborate off list to get this into shape.
Jason A. Donenfeld Sept. 13, 2018, 3:45 p.m. UTC | #9
On Thu, Sep 13, 2018 at 5:04 PM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> > The code still benefits from the review that's gone into OpenSSL. It's

> > not modified in ways that would affect the cryptographic operations

> > being done. It's modified to be suitable for kernel space.

>

> So could we please at least have those changes as a separate patch then?


I'll experiment with a couple ways of trying to communicate with
precision what's been changed from OpenSSL for the next round of
patches.

> > That's interesting. I'll bring this up with AndyP. FWIW, if you think

> > you have a real and compelling claim here, I'd be much more likely to

> > accept a different ChaCha20 implementation than I would be to accept a

> > different Poly1305 implementation. (It's a *lot* harder to screw up

> > ChaCha20 than it is to screw up Poly1305.)

>

> The question is really whether we want different implementations in

> the crypto API and in zinc.


Per earlier in this discussion, I've already authored patches that
replaces the crypto API's implementations with simple calls to Zinc,
so that code isn't duplicated. These will be in v4 and you can comment
on the approach then.

> You are completely missing my point. I am not particularly invested in

> the crypto API, and I share the concerns about its usability. That is

> why I want to make sure that your solution actually results in a net

> improvement for everybody, not just for WireGuard, in a maintainable

> way.


Right, likewise. I've put quite a bit of effort into separating Zinc
into Zinc and not into something part of WireGuard. The motivation for
doing so is a decent amount of call sites all around the kernel that
I'd like to gradually fix up.
Ard Biesheuvel Sept. 14, 2018, 6:15 a.m. UTC | #10
On 13 September 2018 at 17:58, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> On Thu, Sep 13, 2018 at 5:43 PM Ard Biesheuvel

> <ard.biesheuvel@linaro.org> wrote:

>> I'd prefer it if all the accelerated software implementations live in

>> the same place. But I do strongly prefer arch code to live in

>> arch/$arch

>

> Zinc follows the scheme of the raid6 code, as well as of most other

> crypto libraries: code is grouped by cipher, making it easy for people

> to work with and understand differing implementations. It also allows

> us to trivially link these together at compile time rather than at

> link time, which makes cipher selection much more efficient. It's

> really much more maintainable this way.

>

>> I think AES-GCM is a useful example here. I really like the SIMD token

>> abstraction a lot, but I would like to understand how this would work

>> in Zinc if you have

>> a) a generic implementation

>> b) perhaps an arch specific scalar implementation

>> c) a pure NEON implementation

>> d) an implementation using AES instructions but not the PMULL instructions

>> e) an implementation that uses AES and PMULL instructions.

>

> The same way that Zinc currently chooses between the five different

> implementations for, say, x86_64 ChaCha20:

>

> - Generic C scalar

> - SSSE3

> - AVX2

> - AVX512F

> - AVX512VL

>

> We make a decision based on CPU capabilities, SIMD context, and input

> length, and then choose the right function.

>


OK, so given random.c's future dependency on Zinc (for ChaCha20), and
the fact that Zinc is one monolithic piece of code, all versions of
all algorithms will always be statically linked into the kernel
proper. I'm not sure that is acceptable.

>> You know what? If you're up for it, let's not wait until Plumbers, but

>> instead, let's collaborate off list to get this into shape.

>

> Sure, sounds good.

>


BTW you haven't answered my question yet about what happens when the
WireGuard protocol version changes: will we need a flag day and switch
all deployments over at the same time?
Jason A. Donenfeld Sept. 14, 2018, 9:53 a.m. UTC | #11
On Fri, Sep 14, 2018 at 8:15 AM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> OK, so given random.c's future dependency on Zinc (for ChaCha20), and

> the fact that Zinc is one monolithic piece of code, all versions of

> all algorithms will always be statically linked into the kernel

> proper. I'm not sure that is acceptable.


v4 already addresses that issue, actually. I'll post it shortly.

> BTW you haven't answered my question yet about what happens when the

> WireGuard protocol version changes: will we need a flag day and switch

> all deployments over at the same time?


No, that won't be necessary, necessarily. Peers are individually
versioned and the protocol is fairly flexible in this regard.
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 2ef884b883c3..d2092e52320d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16160,6 +16160,14 @@  Q:	https://patchwork.linuxtv.org/project/linux-media/list/
 S:	Maintained
 F:	drivers/media/dvb-frontends/zd1301_demod*
 
+ZINC CRYPTOGRAPHY LIBRARY
+M:	Jason A. Donenfeld <Jason@zx2c4.com>
+M:	Samuel Neves <sneves@dei.uc.pt>
+S:	Maintained
+F:	lib/zinc/
+F:	include/zinc/
+L:	linux-crypto@vger.kernel.org
+
 ZPOOL COMPRESSED PAGE STORAGE API
 M:	Dan Streetman <ddstreet@ieee.org>
 L:	linux-mm@kvack.org
diff --git a/lib/Kconfig b/lib/Kconfig
index a3928d4438b5..3e6848269c66 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -485,6 +485,8 @@  config GLOB_SELFTEST
 	  module load) by a small amount, so you're welcome to play with
 	  it, but you probably don't need it.
 
+source "lib/zinc/Kconfig"
+
 #
 # Netlink attribute parsing support is select'ed if needed
 #
diff --git a/lib/Makefile b/lib/Makefile
index ca3f7ebb900d..3f16e35d2c11 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -214,6 +214,8 @@  obj-$(CONFIG_PERCPU_TEST) += percpu_test.o
 
 obj-$(CONFIG_ASN1) += asn1_decoder.o
 
+obj-$(CONFIG_ZINC) += zinc/
+
 obj-$(CONFIG_FONT_SUPPORT) += fonts/
 
 obj-$(CONFIG_PRIME_NUMBERS) += prime_numbers.o
diff --git a/lib/zinc/Kconfig b/lib/zinc/Kconfig
new file mode 100644
index 000000000000..aa4f8d449d6b
--- /dev/null
+++ b/lib/zinc/Kconfig
@@ -0,0 +1,20 @@ 
+config ZINC
+	tristate
+	select CRYPTO_BLKCIPHER
+	select VFP
+	select VFPv3
+	select NEON
+	select KERNEL_MODE_NEON
+
+config ZINC_DEBUG
+	bool "Zinc cryptography library debugging and self-tests"
+	depends on ZINC
+	help
+	  This builds a series of self-tests for the Zinc crypto library, which
+	  help diagnose any cryptographic algorithm implementation issues that
+	  might be at the root cause of potential bugs. It also adds various
+	  debugging traps.
+
+	  Unless you're developing and testing cryptographic routines, or are
+	  especially paranoid about correctness on your hardware, you may say
+	  N here.
diff --git a/lib/zinc/Makefile b/lib/zinc/Makefile
new file mode 100644
index 000000000000..dad47573de42
--- /dev/null
+++ b/lib/zinc/Makefile
@@ -0,0 +1,8 @@ 
+ccflags-y := -O3
+ccflags-y += -Wframe-larger-than=8192
+ccflags-y += -D'pr_fmt(fmt)=KBUILD_MODNAME ": " fmt'
+ccflags-$(CONFIG_ZINC_DEBUG) += -DDEBUG
+
+zinc-y += main.o
+
+obj-$(CONFIG_ZINC) := zinc.o
diff --git a/lib/zinc/main.c b/lib/zinc/main.c
new file mode 100644
index 000000000000..ceece33ff5a7
--- /dev/null
+++ b/lib/zinc/main.c
@@ -0,0 +1,31 @@ 
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * Copyright (C) 2015-2018 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+
+#ifdef DEBUG
+#define selftest(which) do { \
+	if (!which ## _selftest()) \
+		return -ENOTRECOVERABLE; \
+} while (0)
+#else
+#define selftest(which)
+#endif
+
+static int __init mod_init(void)
+{
+	return 0;
+}
+
+static void __exit mod_exit(void)
+{
+}
+
+module_init(mod_init);
+module_exit(mod_exit);
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Zinc cryptography library");
+MODULE_AUTHOR("Jason A. Donenfeld <Jason@zx2c4.com>");