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:   Mon, 18 Apr 2022 13:46:26 -0700
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: [PATCH v4 3/8] crypto: hctr2 - Add HCTR2 support
Message-ID: <Yl3OIuhhnEN+E9fp@sol.localdomain>
References: <20220412172816.917723-1-nhuck@google.com>
 <20220412172816.917723-4-nhuck@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220412172816.917723-4-nhuck@google.com>
Precedence: bulk

One more comment:

On Tue, Apr 12, 2022 at 05:28:11PM +0000, Nathan Huckleberry wrote:
> +/*
> + * Check for a supported set of inner algorithms.
> + * See the comment at the beginning of this file.
> + */
> +static bool hctr2_supported_algorithms(struct skcipher_alg *xctr_alg,
> +				       struct crypto_alg *blockcipher_alg,
> +				       struct shash_alg *polyval_alg)
> +{
> +	if (strncmp(xctr_alg->base.cra_name, "xctr(", 4) != 0)
> +		return false;
> +
> +	if (blockcipher_alg->cra_blocksize != BLOCKCIPHER_BLOCK_SIZE)
> +		return false;
> +
> +	if (strcmp(polyval_alg->base.cra_name, "polyval") != 0)
> +		return false;
> +
> +	return true;
> +}

There are a couple issues here:

- "See the comment at the beginning of this file" doesn't make sense.  I guess
  this was copied from adiantum.c where there is indeed a comment at the
  beginning of the file that explains which "inner" algorithms are allowed.
  However, in hctr2.c there is no such comment (and that's fine; there aren't as
  many special considerations in this area for hctr2 as for adiantum).

- The strncmp() expression uses a string of 5 characters but only compares 4.
  Also this check is redundant anyway, since hctr2_create_common() already does
  this check (correctly, with 5 characters).

How about deleting the hctr2_supported_algorithms() function and putting the 2
needed checks directly in hctr2_create_common()?  I.e., check
blockcipher_alg->cra_blocksize right after the line:

	blockcipher_alg = crypto_spawn_cipher_alg(&ictx->blockcipher_spawn);

... and check polyval_alg->base.cra_name right after the line:

	polyval_alg = crypto_spawn_shash_alg(&ictx->polyval_spawn);

Note, the pr_warn() message "Unsupported HCTR2 instantiation" isn't very
important, and it arguably shouldn't be there since it is user-triggerable.
So you can just delete it.

- Eric