On Thu, Feb 10, 2022 at 11:28:07PM +0000, Nathan Huckleberry wrote:
> +config CRYPTO_POLYVAL
> + tristate
> + select CRYPTO_GF128MUL
> + select CRYPTO_HASH
> + help
> + POLYVAL is the hash function used in HCTR2. It is not a general-purpose
> + cryptographic hash function.
As with XCTR: as this option is no longer user-selectable, no one will see this
help text. I think it should just be removed.
> +static int polyval_update(struct shash_desc *desc,
> + const u8 *src, unsigned int srclen)
> +{
> + struct polyval_desc_ctx *dctx = shash_desc_ctx(desc);
> + const struct polyval_tfm_ctx *ctx = crypto_shash_ctx(desc->tfm);
> + u8 *dst = dctx->buffer;
The dst variable doesn't seem to serve a purpose. It would be clearer to just
write dctx->buffer directly (or &dctx->buffer[...], etc).
> + u8 *pos;
> + u8 tmp[POLYVAL_BLOCK_SIZE];
> + int n;
> +
> + if (dctx->bytes) {
> + n = min(srclen, dctx->bytes);
> + pos = dst + dctx->bytes - 1;
> +
> + dctx->bytes -= n;
> + srclen -= n;
> +
> + while (n--)
> + *pos-- ^= *src++;
> +
> + if (!dctx->bytes)
> + gf128mul_4k_lle((be128 *)dst, ctx->gf128);
I thought I mentioned this on v1, but the cast to be128 is violating alignment
rules. If the alignment to be128 is needed then a union should be used, e.g.:
struct polyval_desc_ctx {
union {
u8 buffer[POLYVAL_BLOCK_SIZE];
be128 buffer128;
};
u32 bytes;
};
> +static int polyval_final(struct shash_desc *desc, u8 *dst)
> +{
> + struct polyval_desc_ctx *dctx = shash_desc_ctx(desc);
> + const struct polyval_tfm_ctx *ctx = crypto_shash_ctx(desc->tfm);
> + u8 *buf = dctx->buffer;
> +
> + if (dctx->bytes)
> + gf128mul_4k_lle((be128 *)buf, ctx->gf128);
> + dctx->bytes = 0;
> +
> + reverse_block(buf);
> + memcpy(dst, buf, POLYVAL_BLOCK_SIZE);
> +
> + return 0;
> +}
Same issues as polyval_update().
> +
> diff --git a/include/crypto/polyval.h b/include/crypto/polyval.h
> new file mode 100644
> index 000000000000..fd0c6e124b65
> --- /dev/null
> +++ b/include/crypto/polyval.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Common values for the Polyval hash algorithm
> + *
> + * Copyright 2021 Google LLC
> + */
> +
> +#ifndef _CRYPTO_POLYVAL_H
> +#define _CRYPTO_POLYVAL_H
> +
> +#include <linux/types.h>
> +#include <linux/crypto.h>
> +
> +#define POLYVAL_BLOCK_SIZE 16
> +#define POLYVAL_DIGEST_SIZE 16
> +
> +struct polyval_desc_ctx {
> + u8 buffer[POLYVAL_BLOCK_SIZE];
> + u32 bytes;
> +};
> +
> +#endif
As-is, polyval_desc_ctx is only used by crypto/polyval-generic.c, so it
shouldn't be in this header. Either it should be moved to polyval-generic.c, or
all implementations should be made to use the same struct.
- Eric
