On Mon, Mar 25, 2019 at 12:57:20PM +0100, Jason A. Donenfeld wrote:
Hey Herbert,
On Mon, Mar 25, 2019 at 12:52 PM Herbert Xu [off-list ref] wrote:
quoted
Sorry but adding new implementations of chacha20/poly1305 without
removing the existing ones is not acceptable. I really think
we ought to separate the zinc interface from these new crypto
implementations. They have nothing to do with each other.
As we've been stuck on this point for so long, let's get the
ball rolling by first merging just the zinc interface itself
with the existing chacha20/poly1305 code. Then we can replace
these implementations with your implementations without getting
bogged down by all these other discussions.
AFAICS once we resolve Thomas's concerns with regards to the simd
patch, then we can merge the zinc interface right away and go from
there.
I think we're slightly closer to being same page, but I haven't
followed up here yet because I was waiting for something else first.
Your previous patchset left out a few implementations and wasn't
totally complete with regards to the glue code. I'm going to whip
something up now that is Zinc, but where there's already an existing
assembly implementation in the tree (i.e. Martin's code), it uses that
instead of what I've been posting until now. Then, after that lands, I
can post stand-alone patches for replacing the various
implementations, and we can discuss those separately, alongside all
the various discussion of verification and benchmarks and so forth.
So, I'll have something to examine somewhat soon. Working on it now.
Jason
FYI, today I started work on updating testmgr to do fuzz tests where it
generates random test vectors using the generic implementation of each hash,
skcipher, and aead algorithm registered with the crypto API, and tests the other
implementations against them.
poly1305-simd is among the failing algorithms because it loses carry bits when
handling long "all 0xff bytes" inputs. poly1305-avx2-x86_64.S is definitely
broken, and poly1305-sse2-x86_64.S *might* be too. I am working on a patch...
It would be interesting to hear whether the Zinc self-tests detect this too. I
couldn't easily tell from testing Herbert's Zinc patch series, because it seemed
most of the Zinc self-tests were failing for other reasons.
FWIW, this type of thing is one of the reasons we made the performance-critical
part of Adiantum's hash function be NH rather than Poly1305, and for now
crypto/adiantum.c only uses the C implementation of Poly1305. Fast NH
implementations are much simpler and less error-prone than Poly1305. Of course,
proper testing will help a lot...
- Eric