вт, 3 сент. 2019 г. в 20:26, Niels Möller <[email protected]>:
>
> [email protected] writes:
>
> > From: Dmitry Eremin-Solenikov <[email protected]>
> >
> > Move Galois polynomial shifts to block-internal.h, simplifying common
> > code. GCM is left unconverted for now, this will be fixed later.
>
> Thanks for cleaning this up! Some comments below.
>
> > --- a/block-internal.h
> > +++ b/block-internal.h
> > @@ -90,4 +90,80 @@ block8_xor_bytes (union nettle_block8 *r,
> > memxor3 (r->b, x->b, bytes, 8);
> > }
> >
> > +#define LSHIFT_WORD(x) ((((x) & 0x7f7f7f7f7f7f7f7f) << 1) | \
> > + (((x) & 0x8080808080808080) >> 15))
> > +#define RSHIFT_WORD(x) ((((x) & 0xfefefefefefefefe) >> 1) | \
> > + (((x) & 0x0001010101010101) << 15))
>
> Names of these macros should say U64 or UINT64 rather than WORD. And
> something to suggest that they're for alien endianness. Maybe
> "LSHIFT_ALIEN_UINT64.
Ack
> And UINT64_C for the constants.
Ack
> > +/* Galois multiplications by 2:
> > + * functions differ in shifting right or left, big- or little- endianness
> > + * and by defininy polynom.
> > + * r == x is allowed. */
>
> This is a bit complex, perhaps it can be clarified a bit. We have both
> the issue of big or little byte order within words. And bit order used
> for representating of the polynomial: usually a less significant bit
> within a byte represents a coefficient for a smaller power of the
> polynomial variable x, but one of the algorithms (I can't recall which
> one) uses opposite bit order.
For GCM. This is why I left it unconverted in this step.
> And if I remember correctly, they all use the same polynomial, but due
> to bit-order differences, there are two different ways to represent it.
> Which of the functions are called with more than one constant for the
> poly argument?
They take 0x87 for block16 functions and 0x1b for block8. Except GCM,
which uses 0xE1. I will probably inline these values.
> And "defining" is misspelled.
Ack
> > +#if WORDS_BIGENDIAN
> > +static inline void
> > +block16_lshift_be (union nettle_block16 *dst,
> > + const union nettle_block16 *src,
> > + uint64_t poly)
> > +{
> > + uint64_t carry = src->u64[0] >> 63;
> > + dst->u64[0] = (src->u64[0] << 1) | (src->u64[1] >> 63);
> > + dst->u64[1] = (src->u64[1] << 1) ^ (poly & -carry);
> > +}
> > +#else /* !WORDS_BIGENDIAN */
>
> There will be less clutter if all code for #if WORDS_BIGENDIAN is
> grouped together. And I think I prefer "mulx" rather than "shift"
> somewhere in the name, to indicate that it's not a plain shift.
Ack
>
> > --- a/cmac.c
> > +++ b/cmac.c
> > @@ -44,32 +44,16 @@
> >
> > #include "memxor.h"
> > #include "nettle-internal.h"
> > -#include "cmac-internal.h"
> > #include "block-internal.h"
> > #include "macros.h"
> >
> > /* shift one and XOR with 0x87. */
> > -#if WORDS_BIGENDIAN
> > -void
> > -_cmac128_block_mulx(union nettle_block16 *dst,
> > - const union nettle_block16 *src)
> > -{
> > - uint64_t carry = src->u64[0] >> 63;
> > - dst->u64[0] = (src->u64[0] << 1) | (src->u64[1] >> 63);
> > - dst->u64[1] = (src->u64[1] << 1) ^ (0x87 & -carry);
> > -}
> > -#else /* !WORDS_BIGENDIAN */
> > -#define LE_SHIFT(x) ((((x) & 0x7f7f7f7f7f7f7f7f) << 1) | \
> > - (((x) & 0x8080808080808080) >> 15))
> > -void
> > +static inline void
> > _cmac128_block_mulx(union nettle_block16 *dst,
> > const union nettle_block16 *src)
> > {
> > - uint64_t carry = (src->u64[0] & 0x80) >> 7;
> > - dst->u64[0] = LE_SHIFT(src->u64[0]) | ((src->u64[1] & 0x80) << 49);
> > - dst->u64[1] = LE_SHIFT(src->u64[1]) ^ (0x8700000000000000 & -carry);
> > + block16_lshift_be(dst, src, 0x87);
> > }
> > -#endif /* !WORDS_BIGENDIAN */
>
> I think it's clearer to delete this and similar wrappers.
Ack
--
With best wishes
Dmitry
_______________________________________________
nettle-bugs mailing list
[email protected]
http://lists.lysator.liu.se/mailman/listinfo/nettle-bugs
