вт, 3 сент. 2019 г. в 20:26, Niels Möller nisse@lysator.liu.se:
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.
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