Thanks for splitting this out for review. Looks pretty good, a few minor comments below.
Dmitry Baryshkov <[email protected]> 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 -- Niels Möller. PGP-encrypted email is preferred. Keyid 368C6677. Internet email is subject to wholesale government surveillance. _______________________________________________ nettle-bugs mailing list [email protected] http://lists.lysator.liu.se/mailman/listinfo/nettle-bugs
