Re: [PATCH crypto-next v1] crypto: poly1305 - add new 32 and 64-bit generic versions

["Jason A. Donenfeld" <Jason@zx2c4.com>]

Hey Eric,

Thanks for the review. I just finished porting the x86_64 code, which
I'll submit alongside v2 tomorrow, pending a night of sleep and some
rereading: https://git.kernel.org/pub/scm/linux/kernel/git/zx2c4/linux.git/commit/?h=jd/crypto-5.5&id=7380dd3ac2b8ea4a2b1b111139426c7d25374bba

On Wed, Dec 11, 2019 at 8:13 PM Eric Biggers <ebiggers@kernel.org> wrote:
> Isn't the existing implementation in the kernel already based on the 32x32 code
> from Andrew Moon?  Can you elaborate on how the new code is different?

It matches the style of the 64x64 one, so that it's easy to compare
them. And, as mentioned in the commit message, it precomputes
s1,s2,s3,s4, speeding up updates.

> You can run the self-tests.  But I tried and it doesn't get past nhpoly1305:
>
> [    0.856458] alg: shash: nhpoly1305-generic test failed (wrong result) on test vector 1, cfg="init+update+final aligned buffer"

Oh, right, duh. Okay, I'll submit v2 once I get those tests passing. Thanks.

> > +void poly1305_core_emit(const struct poly1305_state *state, const u32 nonce[4],
> > +                     void *dst);
>
> Adding nonce support here makes the comment above this code outdated.
>
>         /*
>          * Poly1305 core functions.  These implement the ε-almost-∆-universal hash
>          * function underlying the Poly1305 MAC, i.e. they don't add an encrypted nonce
>          * ("s key") at the end.  They also only support block-aligned inputs.
>          */

Ack.

> It would be helpful to include comments for the r64 and h64 fields, like there
> are for the r and h fields.  What base is being used?

Sure, I'll add comments.


> This assumes the struct poly1305_key is actually part of an array.  This is
> probably what's causing the self-tests to fail, since some callers provide only
> a single struct poly1305_key.

Thanks. Good catch.

> Shouldn't this detail be hidden in struct poly1305_key?  Or at least the
> functions should take 'struct poly1305_key key[2]' to make this assumption
> explicit.

I'll make it explicit. The way things work right now, the desc_ctx is
an array of a .config-number of keys. In some future cleanup, I think
that should basically be opaque instead, but I'll leave this out of
this now, and instead fix up the signatures to make clear what's going
on, and audit all callers.

Jason