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

[Andy Lutomirski]

> On Sep 11, 2018, at 2:47 PM, Eric Biggers <ebiggers@xxxxxxxxxx> 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