Re: [PATCH 09/21] lib/sha1.c: add file

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.04.2025 17:06, Sergii Dmytruk wrote:
> From: Krystian Hebel <krystian.hebel@xxxxxxxxx>
> 
> The code comes from [1] and is licensed under GPL-2.0 license.
> It's a combination of:
>  - include/crypto/sha1.h
>  - include/crypto/sha1_base.h
>  - lib/crypto/sha1.c
>  - crypto/sha1_generic.c
> 
> Changes:
>  - includes
>  - formatting
>  - renames and splicing of some trivial functions that are called once
>  - dropping of `int` return values (only zero was ever returned)
>  - getting rid of references to `struct shash_desc`

Since you did move the code to (largely) Xen style, a few further requests
in that direction:

> --- /dev/null
> +++ b/xen/include/xen/sha1.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __XEN_SHA1_H
> +#define __XEN_SHA1_H
> +
> +#include <xen/inttypes.h>
> +
> +#define SHA1_DIGEST_SIZE  20
> +
> +void sha1_hash(const u8 *data, unsigned int len, u8 *out);

uint8_t please in both instances here, and more generally {,u}int<N>_t
in place of {s,u}<N>.

> --- a/xen/lib/Makefile
> +++ b/xen/lib/Makefile
> @@ -38,6 +38,7 @@ lib-y += strtoll.o
>  lib-y += strtoul.o
>  lib-y += strtoull.o
>  lib-$(CONFIG_X86) += x86-generic-hweightl.o
> +lib-$(CONFIG_X86) += sha1.o

Please obey to alphabetic sorting.

> --- /dev/null
> +++ b/xen/lib/sha1.c
> @@ -0,0 +1,240 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * SHA1 routine optimized to do word accesses rather than byte accesses,
> + * and to avoid unnecessary copies into the context array.
> + *
> + * This was based on the git SHA1 implementation.
> + */
> +
> +#include <xen/bitops.h>
> +#include <xen/types.h>
> +#include <xen/sha1.h>
> +#include <xen/unaligned.h>
> +
> +/*
> + * If you have 32 registers or more, the compiler can (and should)
> + * try to change the array[] accesses into registers. However, on
> + * machines with less than ~25 registers, that won't really work,
> + * and at least gcc will make an unholy mess of it.
> + *
> + * So to avoid that mess which just slows things down, we force
> + * the stores to memory to actually happen (we might be better off
> + * with a 'W(t)=(val);asm("":"+m" (W(t))' there instead, as
> + * suggested by Artur Skawina - that will also make gcc unable to
> + * try to do the silly "optimize away loads" part because it won't
> + * see what the value will be).
> + *
> + * Ben Herrenschmidt reports that on PPC, the C version comes close
> + * to the optimized asm with this (ie on PPC you don't want that
> + * 'volatile', since there are lots of registers).
> + *
> + * On ARM we get the best code generation by forcing a full memory barrier
> + * between each SHA_ROUND, otherwise gcc happily get wild with spilling and
> + * the stack frame size simply explode and performance goes down the drain.
> + */
> +
> +#ifdef CONFIG_X86
> +  #define setW(x, val) (*(volatile uint32_t *)&W(x) = (val))

The # of pre-processor directives generally wants to be in the first column.

> +#elif defined(CONFIG_ARM)
> +  #define setW(x, val) do { W(x) = (val); __asm__("":::"memory"); } while ( 
> 0 )

__asm__ ( "" ::: "memory" );

as far as style goes. But then I see no need to open-code barrier().

> +#else
> +  #define setW(x, val) (W(x) = (val))
> +#endif
> +
> +/* This "rolls" over the 512-bit array */
> +#define W(x) (array[(x) & 15])
> +
> +/*
> + * Where do we get the source from? The first 16 iterations get it from
> + * the input data, the next mix it from the 512-bit array.
> + */
> +#define SHA_SRC(t) get_unaligned_be32((uint32_t *)data + t)
> +#define SHA_MIX(t) rol32(W(t + 13) ^ W(t + 8) ^ W(t + 2) ^ W(t), 1)

I fear Misra isn't going to like the lack of parenthesization of macro
arguments used in expressions. This looks to be an issue with most
macros here.

> +#define SHA_ROUND(t, input, fn, constant, A, B, C, D, E) do { \
> +        uint32_t TEMP = input(t); setW(t, TEMP);              \
> +        E += TEMP + rol32(A, 5) + (fn) + (constant);          \
> +        B = ror32(B, 2);                                      \
> +        TEMP = E; E = D; D = C; C = B; B = A; A = TEMP;       \
> +    } while ( 0 )
> +
> +#define T_0_15(t, A, B, C, D, E)  \
> +        SHA_ROUND(t, SHA_SRC, (((C ^ D) & B) ^ D), 0x5a827999, A, B, C, D, E)
> +#define T_16_19(t, A, B, C, D, E) \
> +        SHA_ROUND(t, SHA_MIX, (((C ^ D) & B) ^ D), 0x5a827999, A, B, C, D, E)
> +#define T_20_39(t, A, B, C, D, E) \
> +        SHA_ROUND(t, SHA_MIX, (B ^ C ^ D), 0x6ed9eba1, A, B, C, D, E)
> +#define T_40_59(t, A, B, C, D, E)                                            
>  \
> +        SHA_ROUND(t, SHA_MIX, ((B & C) + (D & (B ^ C))), 0x8f1bbcdc, A, B, 
> C, \
> +                  D, E)
> +#define T_60_79(t, A, B, C, D, E) \
> +        SHA_ROUND(t, SHA_MIX, (B ^ C ^ D), 0xca62c1d6, A, B, C, D, E)
> +
> +#define SHA1_BLOCK_SIZE         64
> +#define SHA1_WORKSPACE_WORDS    16
> +
> +struct sha1_state {
> +    uint32_t state[SHA1_DIGEST_SIZE / 4];
> +    uint64_t count;
> +    uint8_t buffer[SHA1_BLOCK_SIZE];
> +};
> +
> +typedef void sha1_block_fn(struct sha1_state *sst, const uint8_t *src, int 
> blocks);

Please respect line length restrictions. The use of plain int here also looks
questionable, as just from the name that parameter looks like it can't have a
negative argument passed for it. This will want adjusting elsewhere as well.

> +/**
> + * sha1_transform - single block SHA1 transform (deprecated)
> + *
> + * @digest: 160 bit digest to update
> + * @data:   512 bits of data to hash
> + * @array:  16 words of workspace (see note)
> + *
> + * This function executes SHA-1's internal compression function.  It updates 
> the
> + * 160-bit internal state (@digest) with a single 512-bit data block (@data).
> + *
> + * Don't use this function.  SHA-1 is no longer considered secure.  And even 
> if
> + * you do have to use SHA-1, this isn't the correct way to hash something 
> with
> + * SHA-1 as this doesn't handle padding and finalization.
> + *
> + * Note: If the hash is security sensitive, the caller should be sure
> + * to clear the workspace. This is left to the caller to avoid
> + * unnecessary clears between chained hashing operations.
> + */
> +void sha1_transform(uint32_t *digest, const uint8_t *data, uint32_t *array)

You add no declaration of this function in the header. Should it be static?
This would also help with the "Don't use ..." part of the comment.

Jan