Thanks for splitting this out for review. Looks pretty good, a few minor comments below.
Dmitry Baryshkov dbaryshkov@gmail.com writes:
--- /dev/null +++ b/streebog.c @@ -0,0 +1,1334 @@ +/* streebog.c - GOST R 34.11-2012 (Streebog) hash function
Would be nice with a reference to an English language spec, both in the file header and the docs (later patch). I take it it's RFC 6986?
+/* Pre-computed results of multiplication of bytes on A and reordered with
- Pi[]. */
+static const uint64_t streebog_table[8][256] = +{
- /* 0 */
- { 0xd01f715b5c7ef8e6ULL, 0x16fa240980778325ULL,
In some ways, UINT64_C(0xd01f715b5c7ef8e6), from stdint.h, is more kosher. But ULL is a bit more readable (IMO), so unless it causes any practical problems on some platform, I think it's fine as is.
+static void +streebog512_compress (struct streebog512_ctx *ctx, const uint8_t *input, size_t count) +{
- uint64_t M[8];
- uint64_t l, cf;
- int i;
- for (i = 0; i < 8; i++, input += 8)
- M[i] = LE_READ_UINT64(input);
- g (ctx->state, M, ctx->count);
- l = ctx->count[0];
- ctx->count[0] += count;
- if (ctx->count[0] < l)
The overflow check could be written
if (ctx->count[0] < count)
and then the local variable l can be deleted. I also think it would be clearer to change the type of count to uint64_t to match the type of ctx->count. Do I get it right, that the count argument always is fairly small?
- { /* overflow */
for (i = 1; i < 8; i++){ctx->count[i]++;if (ctx->count[i] != 0)break;}- }
How far can carry propagate here? If I read it correctly, the count array represents a 512 bit number, initialized to zero. So will be tricky to get test coverage.
- cf = 0;
- ctx->sigma[0] += M[0];
- for (i = 1; i < 8; i++)
- {
if (ctx->sigma[i-1] != M[i-1])- cf = (ctx->sigma[i-1] < M[i-1]);
ctx->sigma[i] += M[i] + cf;- }
This is a bignum addition of the sigma and the M arrays? I think I would write it as something like (untested):
ctx->sigma[0] += M[0]; cf = (ctx->sigma[0] < M[0]); for (i = 1; i < 8; i++) { ctx->sigma[i] += cf; cf = (ctx->sigma[i] < cf); ctx->sigma[i] += M[i]; cf += (ctx->sigma[i] < M[i]); /* |= works fine too */ }
Or maybe with
for (i = 1; i < 7; i++) {...} ctx->sigma[7] += M[7] + cf;
if we want to skip operations for the final carry out.
Maybe with a local variable accumulating the final value for sigma[i], to not have to read and write multiple times (but maybe the compiler will eliminate memory accesses). For reference, the corresponding GMP C loop is at https://gmplib.org/repo/gmp/file/tip/mpn/generic/add_n.c#l37
+static void +streebog512_write_digest(struct streebog512_ctx *ctx,
size_t offset, size_t length,uint8_t *digest)+{
- unsigned i;
- unsigned words;
- unsigned leftover;
- assert(offset + length <= STREEBOG512_DIGEST_SIZE);
- streebog_final(ctx);
- words = length / 8;
- leftover = length % 8;
- for (i = 0; i < words; i++, digest += 8)
- LE_WRITE_UINT64(digest, ctx->state[offset + i]);
- if (leftover)
- {
/* Truncate to the right size */uint64_t word = ctx->state[offset + i] << (8*(8 - leftover));do {- digest[--leftover] = (word >> 56) & 0xff;
- word <<= 8;
} while (leftover);- }
Could this use _nettle_write_le64 instead?
Regards, /Niels