On 28 September 2018 at 07:46, Jason A. Donenfeld [off-list ref] wrote:

Hi Eric,

On Fri, Sep 28, 2018 at 6:55 AM Eric Biggers [off-list ref] wrote:

quoted

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.

I would also strongly prefer that all crypto work is taken through
Herbert's tree, so we have a coherent view of it before it goes
upstream.

quoted

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

Will do.

quoted

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.

quoted

quoted

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.

quoted

quoted

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.

quoted

quoted

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.

quoted

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.

quoted

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.

quoted

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.

As I understood from someone who was at your Kernel Recipes talk, you
mentioned that it actually stands for 'zinc is not crypto/' (note the
slash)

That is needlessly divisive and condescending, so if you want to move
past the rhetoric and the schoolyard nonsense, perhaps you can find a
better name for it? Or just put your stuff into crypto/base/,
crypto/core/ or something like that.