Hello,
чт, 4 июн. 2020 г. в 20:59, Niels Möller <[email protected]>:
>
> 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?
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).
--
With best wishes
Dmitry
_______________________________________________
nettle-bugs mailing list
[email protected]
http://lists.lysator.liu.se/mailman/listinfo/nettle-bugs
