Re: [PATCH net-next v6 00/23] WireGuard: Secure Network Tunnel

[Jason A. Donenfeld]

Hi Eric,

On Fri, Sep 28, 2018 at 6:55 AM Eric Biggers <ebiggers@xxxxxxxxxx> wrote:
> And you still haven't answered my question about adding a new algorithm that is
> useful to have in both APIs.  You're proposing that in most cases the crypto API
> part will need to go through Herbert while the implementation will need to go
> through you/Greg, right?  Or will you/Greg be taking both?  Or will Herbert take
> both?

If an implementation enters Zinc, it will go through my tree. If it
enters the crypto API, it will go through Herbert's tree. If there
wind up being messy tree dependency and merge timing issues, I can
work this out in the usual way with Herbert. I'll be sure to discuss
these edge cases with him when we discuss. I think there's a way to
handle that with minimum friction.

> A documentation file for Zinc is a good idea.  Some of the information in your
> commit messages should be moved there too.

Will do.

> I'm not trying to "politicize" this, but rather determine your criteria for
> including algorithms in Zinc, which you haven't made clear.  Since for the
> second time you've avoided answering my question about whether you'd allow the
> SM4 cipher in Zinc, and you designed WireGuard to be "cryptographically
> opinionated", and you were one of the loudest voices calling for the Speck
> cipher to be removed, and your justification for Zinc complains about cipher
> modes from "90s cryptographers", I think it's reasonable for people to wonder
> whether as the Zinc (i.e. Linux crypto library) maintainer you will restrict the
> inclusion of crypto algorithms to the ones you prefer, rather than the ones that
> users need.  Note that the kernel is used by people all over the world and needs
> to support various standards, protocols, and APIs that use different crypto
> algorithms, not always the ones we'd like; and different users have different
> preferences.  People need to know whether the Linux kernel's crypto library will
> meet (or be allowed to meet) their crypto needs.

WireGuard is indeed quite opinionated in its primitive choices, but I
don't think it'd be wise to apply the same design to Zinc. There are
APIs where the goal is to have a limited set of high-level functions,
and have those implemented in only a preferred set of primitives. NaCl
is a good example of this -- functions like "crypto_secretbox" that
are actually xsalsapoly under the hood. Zinc doesn't intend to become
an API like those, but rather to provide the actual primitives for use
cases where that specific primitive is used. Sometimes the kernel is
in the business of being able to pick its own crypto -- random.c, tcp
sequence numbers, big_key.c, etc -- but most of the time the kernel is
in the business of implementing other people's crypto, for specific
devices/protocols/diskformats. And for those use cases we need not
some high-level API like NaCl, but rather direct access to the
primitives that are required to implement those drivers. WireGuard is
one such example, but so would be Bluetooth, CIFS/SMB, WiFi, and so
on. We're in the business of writing drivers, after all. So, no, I
don't think I'd knock down the addition of a primitive because of a
simple preference for a different primitive, if it was clearly the
case that the driver requiring it really benefited from having
accessible via the plain Zinc function calls. Sorry if I hadn't made
this clear earlier -- I thought Ard had asked more or less the same
thing about DES and I answered accordingly, but maybe that wasn't made
clear enough there.

> > For example, check out the avx blocks function. The radix conversion
> > happens in a few different places throughout. The reason we need it
> > separately here is because, unlike userspace, it's possible the kernel
> > code will transition from 2^26 back to 2^64 as a result of the FPU
> > context changing.
>
> IOW, you had to rewrite the x86 assembly algorithm in C and make it use a
> different Poly1305 context format.  That's a major change, where bugs can be
> introduced -- and at least one was introduced, in fact.  I'd appreciate it if
> you were more accurate in describing your modifications and the potential risks
> involved.

A different Poly1305 context format? Not at all - it's using the exact
same context struct as the assembly. And it's making the same
conversion that the assembly is. There's not something "different"
happening; that's the whole point.

Also, this is not some process of frightfully transcribing assembly to
C and hoping it all works out. This is a careful process of reasoning
about the limbs, proving that the carries are correct, and that the
arithmetic done in C actually corresponds to what we want. And then
finally we check that what we've implemented is indeed the same as
what the assembly implemented. Finally, as I mentioned, hopefully Andy
will be folding this back into his implementations sometime soon
anyway.

> > That's a good idea. I can include some discussion about this as well in
> > the commit message that introduces the glue code, too, I guess? I've
> > been hesitant to fill these commit messages up even more, given there
> > are already so many walls of text and whatnot, but if you think that'd
> > be useful, I'll do that for v7, and also add comments.
>
> Please prefer to put information in the code or documentation rather than in
> commit messages, when appropriate.

Okay, no problem.

> > This is complete and utter garbage, and I find its insinuations insulting
> > and ridiculous. There is absolutely no lack of honesty and no double
> > standard being applied whatsoever. Your attempt to cast doubt about the
> > quality of standards applied and the integrity of the process is wholly
> > inappropriate. When I tell you that high standards were applied and that
> > due-diligence was done in developing a particular patch, I mean what I
> > say.
>
> I
> disagree that my concerns are "complete and utter garbage".  And I think that
> the fact that you prefer to respond by attacking me, rather than committing to
> be more accurate in your claims and to treat all contributions fairly, is
> problematic.

I believe you have the sequence of events wrong. I've stated and made
that there isn't a double standard, that the standards intend to be
evenly rigorous, and I solicited your feedback on how I could best
communicate changes in pre-merged patch series; I did the things
you've said one should do. But your rhetoric then moved to questioning
my integrity, and I believe that was uncalled for, inappropriate, and
not a useful contribution to this mostly productive discussion --
hence garbage. In other words, I provided an acceptable defense, not
an attack. But can we move past all this schoolyard nonsense? Cut the
rhetoric, please; it's really quite overwhelming.

> It's great that you found and fixed this
> bug on your own, and of course that raises my level of confidence in your work.

Good.

> Still, the v6 patchset still includes your claim that "All implementations have
> been extensively tested and fuzzed"; so that claim was objectively wrong.

I don't think that claim is wrong. The fuzzing and testing that's been
done has been extensive, and the fuzzing and testing that continues to
occur is extensive. As mentioned, the bug was fixed pretty much right
after git-send-email. If you'd like I can try to space out the patch
submissions by a little bit longer -- that would probably have various
benefits -- but given that the netdev code is yet to receive a
thorough review, I think we've got a bit of time before this is
merged. The faster-paced patch cycles might inadvertently introduce
things that get fixed immediately after sending then, unfortunately,
but I don't think this will be the case with the final series that's
merged. Though I'm incorporating tons and tons of feedback right now,
I do look forward to the structure of the series settling down a
little bit and the pace of suggestions decreasing, so that I can focus
on auditing and verifying again. But given the dynamism and
interesting insights of the reviews so far, I think the fast pace has
actually been useful for elucidating the various expectations and
suggestions of reviewers. It's most certainly improved this patchset
in terrific ways.

> Well, it doesn't help that you yourself claim that Zinc stands for
> "Zx2c4's INsane Cryptolib"...

This expansion of the acronym was intended as a totally ridiculous
joke. I guess it wasn't received well. I'll remove it from the next
series. Sorry.

Jason