dbaryshkov@gmail.com writes:
From: Dmitry Eremin-Solenikov dbaryshkov@gmail.com
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.
And UINT64_C for the constants.
+/* 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.
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?
And "defining" is misspelled.
+#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.
--- 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.
Regards, /Niels