Hello,
чт, 4 июн. 2020 г. в 20:59, Niels Möller nisse@lysator.liu.se:
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?
Yes, I'll add the reference.
+/* 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?
Yes, count is not greater than 8*64 = 512. Changing it to uint64_t is an interesting idea.
- { /* 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.
Nothing stops one from hashing 4GB array. Or even bigger array.
- 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
OK, I'll rewrite it so, but it gives no speedup on my laptop.
[skipped]
- 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?
Done. I wonder, why I have c&p'd that code (it look so).