Re: [PATCH v2 02/19] crypto: sig - Introduce sig_alg backend

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Tue, 10 Sep 2024 16:30:12 +0200
Lukas Wunner <lukas@wunner.de> wrote:

> Commit 6cb8815f41a9 ("crypto: sig - Add interface for sign/verify")
> began a transition of asymmetric sign/verify operations from
> crypto_akcipher to a new crypto_sig frontend.
> 
> Internally, the crypto_sig frontend still uses akcipher_alg as backend,
> however:
> 
>    "The link between sig and akcipher is meant to be temporary.  The
>     plan is to create a new low-level API for sig and then migrate
>     the signature code over to that from akcipher."
>     https://lore.kernel.org/r/ZrG6w9wsb-iiLZIF@gondor.apana.org.au/
> 
>    "having a separate alg for sig is definitely where we want to
>     be since there is very little that the two types actually share."
>     https://lore.kernel.org/r/ZrHlpz4qnre0zWJO@gondor.apana.org.au/
> 
> Take the next step of that migration and augment the crypto_sig frontend
> with a sig_alg backend to which all algorithms can be moved.
> 
> During the migration, there will briefly be signature algorithms that
> are still based on crypto_akcipher, whilst others are already based on
> crypto_sig.  Allow for that by building a fork into crypto_sig_*() API
> calls (i.e. crypto_sig_maxsize() and friends) such that one of the two
> backends is selected based on the transform's cra_type.
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Hi Lukas,

A few trivial comments.

Jonathan

> diff --git a/crypto/sig.c b/crypto/sig.c
> index 7645bedf3a1f..4f36ceb7a90b 100644
> --- a/crypto/sig.c
> +++ b/crypto/sig.c

> @@ -68,6 +93,14 @@ EXPORT_SYMBOL_GPL(crypto_alloc_sig);
>  
>  int crypto_sig_maxsize(struct crypto_sig *tfm)
>  {
> +	if (crypto_sig_tfm(tfm)->__crt_alg->cra_type != &crypto_sig_type)
> +		goto akcipher;
> +
> +	struct sig_alg *alg = crypto_sig_alg(tfm);
> +
> +	return alg->max_size(tfm);
> +
> +akcipher:

Neat trick for temporary retention of the code.
Hideous code in the meantime ;) Not that I have a better idea.

>  	struct crypto_akcipher **ctx = crypto_sig_ctx(tfm);
>  
>  	return crypto_akcipher_maxsize(*ctx);

> diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> index f02cb075bd68..bb21378aa510 100644
> --- a/crypto/testmgr.c
> +++ b/crypto/testmgr.c
...

> @@ -4317,6 +4324,114 @@ static int alg_test_akcipher(const struct alg_test_desc *desc,
>  	return err;
>  }
>  
> +static int test_sig_one(struct crypto_sig *tfm, const struct sig_testvec *vecs)
> +{
> +	u8 *ptr, *key __free(kfree);
I would move definition of key down to where the constructor is.
Current pattern is fine until some extra code sneaks inbetween with
an error return.

> +	int err, sig_size;
> +
> +	key = kmalloc(vecs->key_len + 2 * sizeof(u32) + vecs->param_len,
> +		      GFP_KERNEL);
> +	if (!key)
> +		return -ENOMEM;

git a/include/crypto/internal/sig.h b/include/crypto/internal/sig.h
> index 97cb26ef8115..b16648c1a986 100644
> --- a/include/crypto/internal/sig.h
> +++ b/include/crypto/internal/sig.h

> +static inline struct crypto_sig *crypto_spawn_sig(struct crypto_sig_spawn
> +								   *spawn)

That's an odd wrap. I'd just go long where this happens and slightly past 80 chars.


> +{
> +	return crypto_spawn_tfm2(&spawn->base);
> +}