Message-ID: <20180927213537.GA27576@zx2c4.com>
Date:   Thu, 27 Sep 2018 23:35:39 +0200
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     Eric Biggers <ebiggers@...nel.org>
Cc:     linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        linux-crypto@...r.kernel.org, davem@...emloft.net,
        gregkh@...uxfoundation.org
Subject: Re: [PATCH net-next v6 00/23] WireGuard: Secure Network Tunnel

Hi Eric,

On Thu, Sep 27, 2018 at 8:29 PM Eric Biggers <ebiggers@...nel.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.

> 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.

> 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.)
  - 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.
  - 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.
  - There's been some considerable computing power sunk into fuzzing it.

There's no doubt about it, we've done our due-diligence here. This is in
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. Of course in the ordinary
course of kernel development, this would exist instead as a standalone commit.
If you have a better idea of how this kind of thing can be communicated, and
where precisely, in the pre-merge process, I'd be interested in hearing
suggestions.

Thanks,
Jason

Powered by blists - more mailing lists