Re: [PATCH v2 09/22] xen/lib: add implementation of SHA-1

[Sergii Dmytruk <sergii.dmytruk@xxxxxxxxx>]

On Wed, May 14, 2025 at 05:58:59PM +0100, Andrew Cooper wrote:
> Please crib from sha2.h as much as you can.  Use xen/types.h, drop the
> double underscore in the guard, and provide a link to the spec.

Until yesterday CODING_STYLE instructed to have 2 underscores, so I
thought sha2.h is outdated.  I'll switch to <xen/types.h>, although it's
overkill there and only grows header dependency tree (it also brings
extra symbols thus hiding missing includes elsewhere).

> I think it's https://csrc.nist.gov/pubs/fips/180-1/final

Looks like https://csrc.nist.gov/pubs/fips/180-4/upd1/final is the
latest for all SHA algorithms.

> > +struct sha1_state {
> > +    uint32_t state[SHA1_DIGEST_SIZE / 4];
> > +    uint64_t count;
> > +    uint8_t buffer[SHA1_BLOCK_SIZE];
> > +};
>
> As it's uint64_t, the count field needs to be first to avoid padding.

Will swap them.

> > +/* This "rolls" over the 512-bit array */
> > +static void set_w(uint32_t w[SHA1_WORKSPACE_WORDS], size_t i, uint32_t val)
> > +{
> > +#ifdef CONFIG_X86
> > +    *(volatile uint32_t *)&w[i & SHA1_WORKSPACE_MASK] = val;
> > +#else
> > +    w[i & SHA1_WORKSPACE_MASK] = val;
> > +# ifdef CONFIG_ARM
> > +    barrier();
> > +# endif
> > +#endif
>
> This is horrible.  I think the problems discussed are created by having
> the loops in sha1_transform() broken in a wrong (read, unhelpful) way.  
> The 5-way shuffle of the chaining variables probably is beyond the
> compilers' ability to unroll given the multiples of 4 currently used.
>
> See the implementation in SKL where I spent a while optimising the code
> generation.  Admittedly that was optimising for size rather than speed,
> but the end result look to be good for both.

I tried just doing regular writes and didn't notice any massive
changes in the disassembly with GCC 14.2.0, in fact the function got
shorter by 41 bytes and its stack frame size decreased by 8 bytes.  That
comment was probably addressing misbehaviour of some older version.

> > +    /* Round 1 - iterations 0-16 take their input from 'data' */
> > +    for ( ; i < 16; ++i ) {
>
> Xen style has this { on the next line.

Will fix.

> > +static void sha1_update(struct sha1_state *sctx, const uint8_t *msg, 
> > size_t len)
> > +{
> > +    unsigned int partial = sctx->count % SHA1_BLOCK_SIZE;
> > +
> > +    sctx->count += len;
> > +
> > +    if ( (partial + len) >= SHA1_BLOCK_SIZE )
> > +    {
> > +        if ( partial )
> > +        {
> > +            int rem = SHA1_BLOCK_SIZE - partial;
>
> Unsigned int please.

Will do.

> > +static void sha1_final(struct sha1_state *sctx, void *out)
>
> Please make this uint8_t digest[SHA1_DIGEST_SIZE] straight away.  This
> was an oversight of mine in sha2-256.c which was fixed when exposing the
> function (c/s aea52ce607fe).

OK.  I skipped that intentionally because it really only adds an
explicit cast in the body which is not much different from casting to
`void *` and back.

Regards