Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 16 Feb 2022 15:16:10 -0800
From:   Eric Biggers <ebiggers@kernel.org>
To:     Nathan Huckleberry <nhuck@google.com>
Cc:     linux-crypto@vger.kernel.org,
        Herbert Xu <herbert@gondor.apana.org.au>,
        "David S. Miller" <davem@davemloft.net>,
        linux-arm-kernel@lists.infradead.org,
        Paul Crowley <paulcrowley@google.com>,
        Sami Tolvanen <samitolvanen@google.com>,
        Ard Biesheuvel <ardb@kernel.org>
Subject: Re: [RFC PATCH v2 2/7] crypto: polyval - Add POLYVAL support
Message-ID: <Yg2FuiT15YwvgRpP@sol.localdomain>
References: <20220210232812.798387-1-nhuck@google.com>
 <20220210232812.798387-3-nhuck@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220210232812.798387-3-nhuck@google.com>
Precedence: bulk

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