Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 27 Sep 2018 18:17:27 -0700
From:   Eric Biggers <ebiggers@kernel.org>
To:     "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc:     linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        linux-crypto@vger.kernel.org, davem@davemloft.net,
        gregkh@linuxfoundation.org
Subject: Re: [PATCH net-next v6 00/23] WireGuard: Secure Network Tunnel
Message-ID: <20180928011726.GA98113@gmail.com>
References: <20180925145622.29959-1-Jason@zx2c4.com>
 <20180927182944.GA22921@gmail.com>
 <20180927213537.GA27576@zx2c4.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180927213537.GA27576@zx2c4.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Sep 27, 2018 at 11:35:39PM +0200, Jason A. Donenfeld wrote:
> Hi Eric,
> 
> On Thu, Sep 27, 2018 at 8:29 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > Why is Herbert Xu's existing crypto tree being circumvented, especially for
> > future patches (the initial merge isn't quite as important as that's a one-time
> > event)?  I like being able to check out cryptodev to test upcoming crypto
> > patches.  And currently, changes to APIs, algorithms, tests, and implementations
> > all go through cryptodev, which is convenient for crypto developers.
> >
> > Apparently, you're proposing that someone adding a new algorithm will now have
> > to submit the API portion to one maintainer (Herbert Xu) and the implementation
> > portion to another maintainer (you), and they'll go through separate git trees.
> > That's inconvenient for developers, and it seems that in practice you and
> > Herbert will be stepping on each other's toes a lot.
> >
> > Can you please reach some kind of sane agreement with Herbert so that the
> > development process isn't fractured into two?  Perhaps you could review patches,
> > but Herbert could still apply them?
> 
> I think you're overthinking it a bit. Zinc will have a few software
> implementations of primitives that are useful in cases where it's nice to call
> the primitive directly. Think: various usages of sha2, siphash, the wireguard
> suite (what this patchset includes), other things in lib/, etc. In so much as
> this winds up duplicating things within the crypto API, I'll work with Herbert
> to build one on top of the other -- as I've done in the two commits in this
> series. But beyond that, think of the two initiatives as orthogonal. I'm
> working on curating a few primitives that are maximally useful throughout
> the kernel for various uses, and doing so in a way that I think brings
> about a certain quality. Meanwhile the crypto API is amassing a huge
> collection of primitives for some things, and that will continue to exist,
> and Herbert will continue to maintain that. I expect for the crossover
> to be fairly isolated and manageable, without too much foreseeable tree-
> conflicts and such. Therefore, Samuel Neves and I plan to maintain the
> codebase we've spent quite some time writing, and maintain our own tree for
> it, which we'll be submitting through Greg. In other words, this is not
> a matter of "circumvention" or "stepping on toes", but rather separate
> efforts. I'm quite certain to the extent they overlap we'll be able to work
> out fairly easily.
> 
> Either way, I'll take your suggestion and reach out to Herbert, since at
> least a discussion between the two of us sounds like it could be productive.

So, Zinc will simultaneously replace the current crypto implementations, *and*
be "orthogonal" and "separate" from all the crypto code currently maintained by
Herbert?  You can't have your cake and eat it too...

I'm still concerned you're splitting the community in two.  It will be unclear
where new algorithms and implementations should go.  Some people will choose
Herbert and the current crypto API and conventions, and some people will choose
you and Zinc...  I still don't see clear guidelines for what will go where.  And
yes, you and Herbert will step on each others' toes and duplicate stuff, as the
efforts are *not* separate, as you've even argued yourself.

Please reach out to Herbert to find a sane solution, ideally one that involves
having a single git tree for crypto development and allows people to continue
crypto development without choosing "sides".

> 
> > I'm also wondering about the criteria for making additions and changes to
> > "Zinc".  You mentioned before that one of the "advantages" of Zinc is that it
> > doesn't include "cipher modes from 90s cryptographers" -- what does that mean
> > exactly?  You've also indicated before that you don't want people modifying the
> > Poly1305 implementations as they are too error-prone.  Useful contributions
> > could be blocked or discouraged in the future. Can you please elaborate on
> > your criteria for contributions to Zinc?
> >
> > Also, will you allow algorithms that aren't up to modern security standards but
> > are needed for compatibility reasons, e.g. MD5, SHA-1, and DES?  There are
> > existing standards, APIs, and data formats that use these "legacy" algorithms;
> > so implementations of them are often still needed, whether we like it or not.
> >
> > And does it matter who designed the algorithms, e.g. do algorithms from Daniel
> > Bernstein get effectively a free pass, while algorithms from certain countries,
> > governments, or organizations are not allowed?  E.g. wireless driver developers
> > may need the SM4 block cipher (which is now supported by the crypto API) as it's
> > specified in a Chinese wireless standard.  Will you allow SM4 in Zinc?  Or will
> > people have to submit some algorithms to Herbert and some to you due to
> > disagreements about what algorithms should be included?
> 
> Similarly here, I think you're over-politicizing everything. Stable address
> generation for IPv6 uses SHA1 -- see net/ipv6/addrconf.c:3203 -- do you think
> that this should use, say, the SM3 chinese hash function instead? No, of
> course not, for a variety of interesting reasons. Rather, it should use some
> simple hash function that's fast in software that we have available in Zinc.
> On the other hand, it seems like parts of the kernel that have pretty high-
> levels of cipher agility -- such as dmcrypt, ipsec, wifi apparently, and
> so on -- will continue to use dynamic-dispatch system like the crypto API,
> since that's what it was made to do and is effective at doing. And so, your
> example of SM4 seems to fit perfectly into what the crypto API is well-suited
> for, and it would fit naturally in there.
> 
> In other words, the "political criteria" for what we add to lib/zinc/ will
> mostly be the same as for the rest of lib/: are there things using it that
> benefit from it being there in a direct and obvious way, and does the
> implementation meet certain quality standards.
> 

So, crypto implementations and algorithms will go to different maintainers,
source locations, and git trees based purely on whether the current users need
"cipher agility"?  Note that usage can change over time; a user that requires a
single cipher could later need multiple, and vice versa.

What if the portion of a wireless driver that needs SM4 doesn't need any other
cipher in the same place, so static dispatch would suffice?  Would SM4 be
allowed in Zinc then?

> > to change them yourself, e.g. when you added the part that converts the
> > accumulator from base 26 to base 32.  I worry there may be double standards
> > here
> 
> We do actually appreciate your concern here. However, there's a lot more that
> went into that short patch than meets the eye:
> 
>   - It matches exactly what Andy Polyakov's code is doing for the exact
>     same reason, so this isn't something that's actually "new". (There
>     are paths inside his implementation that branch from the vector code
>     to the scalar code.)

Matches Andy's code, where?  The reason you had to add the radix conversion is
because his code does *not* handle it...

>   - It has been discussed at length with Andy, including what kinds of
>     proofs we'll need if we want to chop it down further (to remove that
>     final reduction), and why we both don't want to do that yet, and so
>     we go with the full carrying for the avoidance of risk.

Sorry, other people don't know about your private discussions.  For the rest of
us, why not add a comment to the code explaining what's going on?

>   - We've proved its correctness with Z3, actually using an even looser
>     constraint on digit size than what Andy mentioned to us, thus resulting
>     in a stronger proof result. So we're certain this isn't rubbish.

AFAICS actually it *is* rubbish, because your C code stores the accumulator as
64-bit integers whereas the asm code (at least, the 32-bit version) reads it as
32-bit integers.  That won't work correctly on big endian ARM.

>   - There's been some considerable computing power sunk into fuzzing it.
> 
> There's no doubt about it, we've done our due-diligence here. 

Apparently not, given that it's broken on big endian ARM.

> fact the kind of efforts we require of submissions. You could fault us for
> not detailing this in "the commit message" -- except as this is still a
> patch series, we're putting improvements into the 00/XX change log, instead
> of adding fixes and additions on top of the series. 

The details of the correctness proofs and fuzzing you claim to have done aren't
explained, even in the cover letter; so for now we just have to trust you on
that point.  Of course, having bugs in code which you insist was proven correct
+ fuzzed doesn't exactly inspire trust.

I understand that your standards are still as high or even higher than
Herbert's, which is good; crypto code should be held to high standards!  But
based on the evidence, I do worry there's a double standard going on where you
get away with things yourself which you won't allow from others in Zinc.  It's
just not honest, and it will make people not want to contribute to Zinc.
Maintainers are supposed to be unbiased and hold all contributions to the same
standard.

We need "Zinc" to be Linux's crypto library, not "Jason's crypto library".

- Eric