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

["Jarkko Sakkinen" <jarkko@kernel.org>]

On Tue Sep 10, 2024 at 5:30 PM EEST, Lukas Wunner 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>
> ---
>  Documentation/crypto/api-sig.rst      |  15 +++
>  Documentation/crypto/api.rst          |   1 +
>  Documentation/crypto/architecture.rst |   2 +
>  crypto/sig.c                          | 143 +++++++++++++++++++++++++-
>  crypto/testmgr.c                      | 115 +++++++++++++++++++++
>  crypto/testmgr.h                      |  13 +++
>  include/crypto/internal/sig.h         |  80 ++++++++++++++
>  include/crypto/sig.h                  |  61 +++++++++++
>  include/uapi/linux/cryptouser.h       |   5 +
>  9 files changed, 433 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/crypto/api-sig.rst
>
> diff --git a/Documentation/crypto/api-sig.rst b/Documentation/crypto/api-sig.rst
> new file mode 100644
> index 000000000000..e5e87e106884
> --- /dev/null
> +++ b/Documentation/crypto/api-sig.rst
> @@ -0,0 +1,15 @@
> +Asymmetric Signature Algorithm Definitions
> +------------------------------------------
> +
> +.. kernel-doc:: include/crypto/sig.h
> +   :functions: sig_alg
> +
> +Asymmetric Signature API
> +------------------------
> +
> +.. kernel-doc:: include/crypto/sig.h
> +   :doc: Generic Public Key Signature API
> +
> +.. kernel-doc:: include/crypto/sig.h
> +   :functions: crypto_alloc_sig crypto_free_sig crypto_sig_set_pubkey crypto_sig_set_privkey crypto_sig_maxsize crypto_sig_sign crypto_sig_verify
> +
> diff --git a/Documentation/crypto/api.rst b/Documentation/crypto/api.rst
> index ff31c30561d4..8b2a90521886 100644
> --- a/Documentation/crypto/api.rst
> +++ b/Documentation/crypto/api.rst
> @@ -10,4 +10,5 @@ Programming Interface
>     api-digest
>     api-rng
>     api-akcipher
> +   api-sig
>     api-kpp
> diff --git a/Documentation/crypto/architecture.rst b/Documentation/crypto/architecture.rst
> index 646c3380a7ed..15dcd62fd22f 100644
> --- a/Documentation/crypto/architecture.rst
> +++ b/Documentation/crypto/architecture.rst
> @@ -214,6 +214,8 @@ the aforementioned cipher types:
>  
>  -  CRYPTO_ALG_TYPE_AKCIPHER Asymmetric cipher
>  
> +-  CRYPTO_ALG_TYPE_SIG Asymmetric signature
> +
>  -  CRYPTO_ALG_TYPE_PCOMPRESS Enhanced version of
>     CRYPTO_ALG_TYPE_COMPRESS allowing for segmented compression /
>     decompression instead of performing the operation on one segment

I'd split the documentation update. It's not strictly necessary as it is
still part of crypto (e.g. not kernel-parameters.txt) but they are still
too disjoint logical artifacts that you need to review separately.

> diff --git a/crypto/sig.c b/crypto/sig.c
> index 7645bedf3a1f..4f36ceb7a90b 100644
> --- a/crypto/sig.c
> +++ b/crypto/sig.c
> @@ -21,14 +21,38 @@
>  
>  static const struct crypto_type crypto_sig_type;
>  
> +static void crypto_sig_exit_tfm(struct crypto_tfm *tfm)
> +{
> +	struct crypto_sig *sig = __crypto_sig_tfm(tfm);
> +	struct sig_alg *alg = crypto_sig_alg(sig);
> +
> +	alg->exit(sig);
> +}
> +
>  static int crypto_sig_init_tfm(struct crypto_tfm *tfm)
>  {
>  	if (tfm->__crt_alg->cra_type != &crypto_sig_type)
>  		return crypto_init_akcipher_ops_sig(tfm);
>  
> +	struct crypto_sig *sig = __crypto_sig_tfm(tfm);
> +	struct sig_alg *alg = crypto_sig_alg(sig);
> +
> +	if (alg->exit)
> +		sig->base.exit = crypto_sig_exit_tfm;
> +
> +	if (alg->init)
> +		return alg->init(sig);

1. alg->exit == NULL, alg->init == NULL
2. alg->exit != NULL, alg->init == NULL
3. alg->exit == NULL, alg->init != NULL

Which of the three are legit use of the API and which are not?

BR, Jarkko