| Message ID | 20180824213849.23647-3-Jason@zx2c4.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | None | expand |
Hi Jason, On Fri, Aug 24, 2018 at 03:38:34PM -0600, Jason A. Donenfeld wrote: > 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 I think the crypto portion of the patchset is looking *slightly* better from v1. (Thanks for getting rid of the #ifdef mazes!) But here are some more comments, with the caveat that I haven't really reviewed any actual implementations yet, and it's a *lot* of new code so this is really just scratching the surface...: I thought you were going to wrap lines at 80 characters? It's hard to read the extremely long lines, and they encourage deep nesting. As I said before, I still think you need to switch the crypto API ChaCha20 and Poly1305 over to use the new implementations. It's not okay to have two completely different sets of ChaCha20 and Poly1305 implementations just because you want a different API, so you might as well get started on it... The thing is that before you try it, it's not clear what problems will come up that require changes to the design. So, this really ought to be addressed up-front. It's also not clearly explained whether/why/how the new ChaCha20 and Poly1305 implementations are better than the existing ones. The patch adding the ARM and ARM64 ChaCha, for example, just says who wrote them, with no mention of why the particular implementations were chosen. You've also documented a lot of stuff in commit messages which will be lost as it isn't being added to the source itself. There are various examples of this, such as information about where the various implementations came from, what you changed, why a particular implementation isn't used on Skylake or whatever, etc. Can you please make sure that any important information is in comments, e.g. at the top of the files? There maybe should even be a Documentation/ file for "Zinc", rather than only a long commit message. I still think the "Zinc" name is confusing and misleading, and people are going to forget that it means "crypto". lib/crypto/ would be more intuitive. But I don't care *that* much myself, and you should see what others think... It seems you still don't explicitly clarify anywhere in the source itself that the copyright holders of the code from OpenSSL have relicensed it under GPLv2. I only see a GPLv2 license slapped on the files, yet no such license is presence in the OpenSSL originals, at least in the one I checked. If you did receive explicit permission, then you should include an explicit clarification in each file like the one in arch/arm/crypto/sha1-armv4-large.S. Otherwise people will be confused and come asking about the license status. As Ard and I discussed recently on my patch "crypto: arm/poly1305 - add NEON accelerated Poly1305 implementation" which proposed adding the exact same poly1305-arm.S file, for all the OpenSSL assembly it would probably be better to include the .pl file and generate the .S file as part of the build process. For one, there is semantic information like register names in the .pl script that is lost in the .S file, thereby making the .S file less readable. There are still some alignment bugs where integers are loaded from byte arrays without using the unaligned access macros, e.g. in chacha20_init(), hchacha20_generic(), and fe_frombytes_impl(). I found these grepping for le32_to_cpu. Interestingly, that last one is in "formally verified" code :-) Thanks! Eric
Hi Jason, On 24 August 2018 at 22:38, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > 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. > Do we really need a new crypto API for WireGuard? Surely, you yourself are not affected by these concerns, and I don't anticipate an explosion of new crypto use cases in the kernel that would justify maintaining two parallel crypto stacks. Also, I take it this means that WireGuard will only work with your routines? We don't usually impose that kind of policy in the kernel: on my arm64 systems, AES/GCM runs at 2.3 cycles per byte, which is a good enough reason to prefer it over a ChaCha20/Poly1305 based AEAD, even if your code is faster than the current code (which is debatable, as you know) It also means that users of the crypto API will not benefit from your better code. I'm sure you agree that it is a bad idea to force those same crypto-impaired programmers to port their code to a new API. I also suggested that we work with Andy Polyakov (as I have in the past) to make the changes required for the kernel in the OpenSSL upstream. That way, we can take the .pl files unmodified, and benefit from the maintenance and review upstream. Dumping 10,000s of lines of generated assembler in the kernel tree like that is really unacceptable IMO. I think you will have to engage with the kernel community to identify the problems with the current crypto API, and get them fixed. I think it makes a lot of sense to have crypto primitives in lib/ (and arch-specific accelerated versions in arch/<arch>/lib), and layer many of the current crypto API drivers on top of that. Also, having more test cases is useful as well. I'm not sure what the relevance of formally verified implementations is, though, since it only proves that the code is true to the algorithm, not that the algorithm is secure. Or am I missing something? Upstreaming your code is a lot easier if you don't cater for your own needs only but also for the needs of others. Please work with us to fix the problems in the current crypto API before parachuting in a new one. > 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 | 7 +++++++ > lib/zinc/main.c | 31 +++++++++++++++++++++++++++++++ > 6 files changed, 70 insertions(+) > create mode 100644 lib/zinc/Kconfig > create mode 100644 lib/zinc/Makefile > create mode 100644 lib/zinc/main.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 955463f8d518..f194eda86011 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -16103,6 +16103,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 706836ec314d..7ea5437e9f7d 100644 > --- a/lib/Kconfig > +++ b/lib/Kconfig > @@ -478,6 +478,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 d95bb2525101..ee41151cba7a 100644 > --- a/lib/Makefile > +++ b/lib/Makefile > @@ -212,6 +212,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..8e30115217db > --- /dev/null > +++ b/lib/zinc/Makefile > @@ -0,0 +1,7 @@ > +ccflags-y := -O3 > +ccflags-y += -Wframe-larger-than=8192 > +ccflags-y += -D'pr_fmt(fmt)=KBUILD_MODNAME ": " fmt' > + > +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..590872563955 > --- /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 CONFIG_ZINC_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>"); > -- > 2.18.0 >
> It seems you still don't explicitly clarify anywhere in the source itself that > the copyright holders of the code from OpenSSL have relicensed it under GPLv2. > I only see a GPLv2 license slapped on the files, yet no such license is presence > in the OpenSSL originals, at least in the one I checked. If you did receive > explicit permission, then you should include an explicit clarification in each > file like the one in arch/arm/crypto/sha1-armv4-large.S. Better yet, get the copyright holders to publicly send a signed-off-by: or acked-by: so it is clear they agree to this. Andrew
Hey Eric, On Fri, Aug 24, 2018 at 11:29:52PM -0700, Eric Biggers wrote: > I thought you were going to wrap lines at 80 characters? It's hard to read the > extremely long lines, and they encourage deep nesting. I somehow noted this for the WireGuard side of things but assumed I didn't need to do so for Zinc. Hah, such wishful thinking. I'll have this wrapped at 80 for v3. > As I said before, I still think you need to switch the crypto API ChaCha20 and > Poly1305 over to use the new implementations. It's not okay to have two > completely different sets of ChaCha20 and Poly1305 implementations just because > you want a different API, so you might as well get started on it... The thing > is that before you try it, it's not clear what problems will come up that > require changes to the design. So, this really ought to be addressed up-front. It was my hope that this entire series could enter the tree through Dave's net-next, and that I wouldn't have to touch anything in crypto/ or touch any of Herbert's stuff at all in anyway. However, if you want, I can start to play with that in another branch for a separate patchset, and of course I'd really value your feedback on that and on doing it right. > It's also not clearly explained whether/why/how the new ChaCha20 and Poly1305 > implementations are better than the existing ones. The patch adding the ARM and > ARM64 ChaCha, for example, just says who wrote them, with no mention of why the > particular implementations were chosen. I can expand on that, sure. One primary advantage that I do touch on on the big introductory commit message, though, is that sharing code means that we benefit from the eyeballs and fuzzing hours spent on OpenSSL, and I think this general advantage is extremely compelling. > You've also documented a lot of stuff in commit messages which will be lost as > it isn't being added to the source itself. There are various examples of this, > such as information about where the various implementations came from, what you > changed, why a particular implementation isn't used on Skylake or whatever, etc. > Can you please make sure that any important information is in comments, e.g. at > the top of the files? Good idea. I'll do that for v3. > I still think the "Zinc" name is confusing and misleading, and people are going > to forget that it means "crypto". lib/crypto/ would be more intuitive. > But I don't care *that* much myself, and you should see what others think... I'd like to keep it. It also enables a natural path for a corresponding userspace library that shares all of the same code, and one that I think will be compelling to attract academics and developers to contributing to these efforts. Plus, can't I name what I made? > It seems you still don't explicitly clarify anywhere in the source itself that > the copyright holders of the code from OpenSSL have relicensed it under GPLv2. > I only see a GPLv2 license slapped on the files, yet no such license is presence > in the OpenSSL originals, at least in the one I checked. The SPDX headers for those came out of a discussion on how to encode CRYPTOGAMS into SPDX from the SPDX mailing list several months ago. > If you did receive > explicit permission, then you should include an explicit clarification in each > file like the one in arch/arm/crypto/sha1-armv4-large.S. That's a good idea. > As Ard and I discussed recently on my patch > "crypto: arm/poly1305 - add NEON accelerated Poly1305 implementation" > which proposed adding the exact same poly1305-arm.S file, for all the OpenSSL > assembly it would probably be better to include the .pl file and generate the .S > file as part of the build process. For one, there is semantic information like > register names in the .pl script that is lost in the .S file, thereby making the > .S file less readable. But on the other hand, the .S files have been modified and changed to fit kernel constraints and conventions. They're very much no longer merely "generated". This is most apparent with the x86_64 files, but is present too with the ARM files. We're still, I believe, bug-for-bug similar to the originals -- keeping with the point of going with Andy's implementations in the first place -- and we've been able to pretty trivially track OpenSSL changes -- but nonetheless important tweaks have been done to make this suitable to kernel space. > There are still some alignment bugs where integers are loaded from byte arrays > without using the unaligned access macros, e.g. in chacha20_init(), > hchacha20_generic(), and fe_frombytes_impl(). I found these grepping for > le32_to_cpu. Interestingly, that last one is in "formally verified" code :-) Thanks. I'll do another pass at these for v3. Jason
Hello Ard, On Sat, Aug 25, 2018 at 11:17:42AM +0100, Ard Biesheuvel wrote: > Do we really need a new crypto API for WireGuard? Surely, you yourself > are not affected by these concerns, and I don't anticipate an > explosion of new crypto use cases in the kernel that would justify > maintaining two parallel crypto stacks. Yes, we really do. And the new API won't be useful for WireGuard but also for the majority of cryptography users within the kernel, which will gradually be ported to use the new API. > Also, I take it this means that WireGuard will only work with your > routines? We don't usually impose that kind of policy in the kernel: > on my arm64 systems, AES/GCM runs at 2.3 cycles per byte, which is a > good enough reason to prefer it over a ChaCha20/Poly1305 based AEAD, > even if your code is faster than the current code (which is debatable, > as you know) WireGuard is a protocol that specifies ChaPoly, and I am proposing an implementation of that protocol. The protocol explicitly does not specify other ciphers. Please read the whitepaper for extensive discussion and feel free to come on over to wireguard@lists.zx2c4.com if you really want to hardcore bikeshed on protocol particulars. The short answer, however, is: no WireGuard won't be adding cipher negotiation, and no, this isn't considered a good feature, and no, you won't have any luck changing that. I am certain, though, that pushing this point will be a terrific means of steering this implementation thread far off course, as various folks with their various opinions feel compelled to jump in about their thoughts on cipher negotiation. Yawn. Besides, your claim about "impos[ing] that kind of policy in the kernel" is bogus: with the exception of IPsec, dm-crypt, and AF_ALG, basically all users of cryptography I can find are using a very particular set of ciphers. For example, Bluetooth uses a particular elliptic curve that has been specified by the Bluetooth people, and the kernel therefore implements a driver that does computations over that elliptic curve. Nobody on LKML is clamoring for that driver to also support Curve448 or something, since that's not what Bluetooth specifies or how Bluetooth works. So too, that's not what WireGuard specifies or how WireGuard works. > It also means that users of the crypto API will not benefit from your > better code. I'm sure you agree that it is a bad idea to force those > same crypto-impaired programmers to port their code to a new API. As mentioned, I think we'll certainly be gradually porting existing crypto API users over to the new API where appropriate. As always, this is a gradual thing, and won't ever come all at once in the first patchset, but I'm pretty confident we can get there somewhat quickly, especially as new primitives become available in the new API. > I also suggested that we work with Andy Polyakov (as I have in the > past) to make the changes required for the kernel in the OpenSSL > upstream. That way, we can take the .pl files unmodified, and benefit > from the maintenance and review upstream. Dumping 10,000s of lines of > generated assembler in the kernel tree like that is really > unacceptable IMO. I'm happy to do that, and indeed I do enjoy working with Andy. However, I don't think your characterization of the current situation is at all accurate. Rather, those .S files have been extensively worked with and crafted. They're far from being merely generated, and they've been pretty heavily reviewed. > I'm not sure what the relevance of > formally verified implementations is, though, since it only proves > that the code is true to the algorithm, not that the algorithm is > secure. Or am I missing something? You are indeed missing something. When the code is not true to the algorithm, that is very bad. When the code is true to the algorithm, that is very good. Formally verified implementations intend to increase our confidence in the latter. Meanwhile on the algorithm side of thing, the WireGuard protocol has a number of proofs that the protocol is secure: - https://www.wireguard.com/papers/wireguard-formal-verification.pdf - https://www.wireguard.com/papers/dowling-paterson-computational-2018.pdf - https://www.wireguard.com/formal-verification/ Before you get too excited though, keep in mind that this is a proof of the protocol, and not of each primitive, like, say, "ChaCha20" or "Curve25519." However, academic literature is full of all sorts of curious and fascinating security analyses of the various primitives if this is something you're interested in. Jason
Pressed send too fast. On Sat, Aug 25, 2018 at 11:06 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > - https://www.wireguard.com/papers/wireguard-formal-verification.pdf > - https://www.wireguard.com/papers/dowling-paterson-computational-2018.pdf > - https://www.wireguard.com/formal-verification/ - https://www.wireguard.com/papers/lipp-computational-2018.pdf - https://eprint.iacr.org/2018/766.pdf
On Sat, Aug 25, 2018 at 10:40:06AM -0600, Jason A. Donenfeld wrote: > Hey Eric, > > On Fri, Aug 24, 2018 at 11:29:52PM -0700, Eric Biggers wrote: > > I thought you were going to wrap lines at 80 characters? It's hard to read the > > extremely long lines, and they encourage deep nesting. > > I somehow noted this for the WireGuard side of things but assumed I > didn't need to do so for Zinc. Hah, such wishful thinking. I'll have > this wrapped at 80 for v3. Hi Jason The coding style document applies to all code in the tree. Plus some out of tree stuff, like iproute2 if, you need user space patches for that. I've been running checkpatch on just the network parts. But i expect at some point, somebody will run it on the crypto stuff as well, and ask you to fix up some of the errors and warning. There is also a general trend that you won't get in detail human reviews until the automated review tools indicate the code is O.K., Andrew
On Sat, Aug 25, 2018 at 12:29 AM Eric Biggers <ebiggers@kernel.org> wrote: > I thought you were going to wrap lines at 80 characters? It's hard to read the > extremely long lines, and they encourage deep nesting. > There are still some alignment bugs where integers are loaded from byte arrays > without using the unaligned access macros, e.g. in chacha20_init(), > hchacha20_generic(), and fe_frombytes_impl(). These fixes are now completed in the development tree.
diff --git a/MAINTAINERS b/MAINTAINERS index 955463f8d518..f194eda86011 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -16103,6 +16103,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 706836ec314d..7ea5437e9f7d 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -478,6 +478,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 d95bb2525101..ee41151cba7a 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -212,6 +212,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..8e30115217db --- /dev/null +++ b/lib/zinc/Makefile @@ -0,0 +1,7 @@ +ccflags-y := -O3 +ccflags-y += -Wframe-larger-than=8192 +ccflags-y += -D'pr_fmt(fmt)=KBUILD_MODNAME ": " fmt' + +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..590872563955 --- /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 CONFIG_ZINC_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>");
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 | 7 +++++++ lib/zinc/main.c | 31 +++++++++++++++++++++++++++++++ 6 files changed, 70 insertions(+) create mode 100644 lib/zinc/Kconfig create mode 100644 lib/zinc/Makefile create mode 100644 lib/zinc/main.c