Re: [PATCH crypto-next v3 1/3] crypto: poly1305 - add new 32 and 64-bit generic versions

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Dec 13, 2019 at 4:03 AM Eric Biggers <ebiggers@xxxxxxxxxx> wrote:
>
> On Thu, Dec 12, 2019 at 06:32:56PM +0100, Jason A. Donenfeld wrote:
> > diff --git a/include/crypto/internal/poly1305.h b/include/crypto/internal/poly1305.h
> > index 479b0cab2a1a..ad97819711eb 100644
> > --- a/include/crypto/internal/poly1305.h
> > +++ b/include/crypto/internal/poly1305.h
> > @@ -11,11 +11,12 @@
> >  #include <crypto/poly1305.h>
> >
> >  /*
> > - * Poly1305 core functions.  These implement the ε-almost-∆-universal hash
> > + * Poly1305 core functions.  These can 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.
> > + * ("s key") at the end, by passing NULL to nonce.  They also only support
> > + * block-aligned inputs.
> >   */
>
> This comment is now ambiguous because the "i.e." clause wasn't updated to match
> the change "implement" => "can implement".
>
> This comment also wasn't updated when the 'hibit' argument was added earlier
> anyway.  So how about just rewriting it like:
>
> /*
>  * Poly1305 core functions.  These only accept whole blocks; the caller must
>  * handle any needed block buffering and padding.  'hibit' must be 1 for any
>  * full blocks, or 0 for the final block if it had to be padded.  If 'nonce' is
>  * non-NULL, then it's added at the end to compute the Poly1305 MAC.  Otherwise,
>  * only the ε-almost-∆-universal hash function (not the full MAC) is computed.
>  */
>

Sounds good.

> > diff --git a/include/crypto/nhpoly1305.h b/include/crypto/nhpoly1305.h
> > index 53c04423c582..a2f4e37b5107 100644
> > --- a/include/crypto/nhpoly1305.h
> > +++ b/include/crypto/nhpoly1305.h
> > @@ -33,7 +33,7 @@
> >  #define NHPOLY1305_KEY_SIZE  (POLY1305_BLOCK_SIZE + NH_KEY_BYTES)
> >
> >  struct nhpoly1305_key {
> > -     struct poly1305_key poly_key;
> > +     struct poly1305_key poly_key[2];
> >       u32 nh_key[NH_KEY_WORDS];
> >  };
>
> I still feel that this makes the code worse.  Before, poly1305_key was an opaque
> type that represented a Poly1305 key.  After, users would need to know that an
> array of 2 "keys" is needed, despite there actually being only one key.
>
> Given that this even caused an actual bug in v1 of this series, how about going
> with a less error-prone approach?
>
> > +void poly1305_core_blocks(struct poly1305_state *state,
> > +                       const struct poly1305_key *key, const void *src,
> > +                       unsigned int nblocks, u32 hibit)
> > +{
> > +     const u8 *input = src;
> > +     u32 r0, r1, r2, r3, r4;
> > +     u32 s1, s2, s3, s4;
> > +     u32 h0, h1, h2, h3, h4;
> > +     u64 d0, d1, d2, d3, d4;
> > +     u32 c;
> > +
> > +     if (!nblocks)
> > +             return;
> > +
> > +     hibit <<= 24;
> > +
> > +     r0 = key[0].r[0];
> > +     r1 = key[0].r[1];
> > +     r2 = key[0].r[2];
> > +     r3 = key[0].r[3];
> > +     r4 = key[0].r[4];
> > +
> > +     s1 = key[1].r[0];
> > +     s2 = key[1].r[1];
> > +     s3 = key[1].r[2];
> > +     s4 = key[1].r[3];
>
> And some of the function prototypes, like this one, still give no indication
> that 2 "keys" are needed...

I'll try to fix it up a bit using the type system.




[Index of Archives]     [Kernel]     [Gnu Classpath]     [Gnu Crypto]     [DM Crypt]     [Netfilter]     [Bugtraq]

  Powered by Linux