Re: [RFC PATCH 2/7] crypto: polyval - Add POLYVAL support

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>
Subject: Re: [RFC PATCH 2/7] crypto: polyval - Add POLYVAL support
Date: Wed, 26 Jan 2022 21:19:27 -0800	[thread overview]
Message-ID: <YfIrXzoKsX5TjAGY@sol.localdomain> (raw)
In-Reply-To: <20220125014422.80552-3-nhuck@google.com>

On Mon, Jan 24, 2022 at 07:44:17PM -0600, Nathan Huckleberry wrote:
> Add support for POLYVAL, an ε-universal hash function similar to GHASH.

I think you mean ε-∆U (i.e. ε-∆-universal), as appears elsewhere in this
patchset?

> POLYVAL is used as a component to implement HCTR2 mode.
> 
> POLYVAL is implemented as an shash algorithm.  The implementation is
> modified from ghash-generic.c.
> 
> More information on POLYVAL can be found in the HCTR2 paper:
> https://eprint.iacr.org/2021/1441.pdf
> 
> Signed-off-by: Nathan Huckleberry <nhuck@google.com>

This commit message could use a brief mention of why POLYVAL is used instead of
GHASH, and where POLYVAL is originally from.  It is in the paper, but it's worth
emphasizing.

> diff --git a/crypto/polyval-generic.c b/crypto/polyval-generic.c
> new file mode 100644
> index 000000000000..63e908697ea0
> --- /dev/null
> +++ b/crypto/polyval-generic.c
> @@ -0,0 +1,183 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * POLYVAL: hash function for HCTR2.
> + *
> + * Copyright (c) 2007 Nokia Siemens Networks - Mikko Herranen <mh1@iki.fi>
> + * Copyright (c) 2009 Intel Corp.
> + *   Author: Huang Ying <ying.huang@intel.com>
> + * Copyright 2021 Google LLC
> + */
> +
> +/*
> + * Code based on crypto/ghash-generic.c
> + *
> + * POLYVAL is a keyed hash function similar to GHASH. POLYVAL uses a
> + * different modulus for finite field multiplication which makes hardware
> + * accelerated implementations on little-endian machines faster.
> + *
> + * Like GHASH, POLYVAL is not a cryptographic hash function and should
> + * not be used outside of crypto modes explicitly designed to use POLYVAL.
> + *
> + */

This comment could use some more explanation about the implementation.  The code
is using the implementation trick where the multiplication is actually done
using the GHASH field, but it is not explained.  Also, it should be explained
why this implementation was chosen.  The reason that the GHASH trick is used
instead of doing a POLYVAL native implementation is because in practice, one of
the accelerated implementations will/should be used instead, right?  So this one
didn't matter much -- there just had to be a generic implementation.

There should also be a warning that this implementation isn't constant-time.

- Eric 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel