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
