RE: [EXT] [RFC] cryptodev: refactor sm2, add plain message flag

[Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>]

Hi,
Please find my comments inline. At the same time, may I request you to 
review my patch series :
https://patches.dpdk.org/project/dpdk/list/?series=29149

> SM2 asymmetric crypto operation was split into cipher and signature
> operation. Now it corresponds to the other crypto algorithms and facilitates
> addition of other SM2 components like the SM2 key exchange.
> 
> Flag to distinguish between a plain message or a hash used for signature was
> added to the DSA, ECDSA and SM2.
> 
> Signed-off-by: Arkadiusz Kusztal <arkadiuszx.kusztal@intel.com>
> ---
>  lib/cryptodev/rte_crypto_asym.h | 116 +++++++++++++++++---------------
>  1 file changed, 63 insertions(+), 53 deletions(-)
> 
> diff --git a/lib/cryptodev/rte_crypto_asym.h
> b/lib/cryptodev/rte_crypto_asym.h index 8b5794fb7c..43bdb392c5 100644
> --- a/lib/cryptodev/rte_crypto_asym.h
> +++ b/lib/cryptodev/rte_crypto_asym.h
> @@ -54,6 +54,7 @@ rte_crypto_asym_op_strings[];
>   * and if the flag is not set, shared secret will be padded to the left with
>   * zeros to the size of the underlying algorithm (default)
>   */
> +#define RTE_CRYPTO_ASYM_FLAG_PLAIN_INPUT		RTE_BIT32(2)
> 
>  /**
>   * List of elliptic curves. This enum aligns with @@ -379,16 +380,6 @@ struct
> rte_crypto_ec_xform {
>  	/**< Pre-defined ec groups */
>  };
> 
> -/**
> - * Asymmetric SM2 transform data.
> - *
> - * Structure describing SM2 xform params.
> - */
> -struct rte_crypto_sm2_xform {
> -	enum rte_crypto_auth_algorithm hash;
> -	/**< Hash algorithm used in SM2 op. */
> -};
> -
>  /**
>   * Operations params for modular operations:
>   * exponentiation and multiplicative inverse @@ -540,7 +531,12 @@ struct
> rte_crypto_dsa_op_param {
>  	enum rte_crypto_asym_op_type op_type;
>  	/**< Signature Generation or Verification */
>  	rte_crypto_param message;
> -	/**< input message to be signed or verified */
> +	/**<
> +	 * Pointer to the input data
> +	 * In case RTE_CRYPTO_ASYM_FLAG_PLAIN_INPUT flag is set in the
> op flags field,
> +	 * it is a message to be signed by the PMD.
> +	 * Otherwise, it is a message hash.
> +	 */

If a PMD does not support plain message but only hash digest, then this new flag
cannot help, as it is set by application without checking PMD capabilities. 
Instead, I had proposed adding hash capability for any EC xform in general
(as other EC curves too can benefit out of it).
https://patches.dpdk.org/project/dpdk/patch/086351e84370ce65dcf947dba12a46f9c62ae79b.1691658879.git.gmuthukrishn@marvell.com/

To note, more than one hash algorithm needs to be supported as in ECDSA for eg. so I made it bitmask of hash algorithms supported by PMD.
For SM2, today we set only SM3.

With this, the application can check the xform capability and set op params as shown in :
https://patches.dpdk.org/project/dpdk/patch/f3be0a425170ee26a1396d34f52a8e07941f7ce5.1691658879.git.gmuthukrishn@marvell.com/

>  	rte_crypto_uint k;
>  	/**< Per-message secret number, which is an integer
>  	 * in the interval (1, q-1).
> @@ -579,7 +575,12 @@ struct rte_crypto_ecdsa_op_param {
>  	/**< Public key of the signer for verification */
> 
>  	rte_crypto_param message;
> -	/**< Input message digest to be signed or verified */
> +	/**<
> +	 * Pointer to the input data
> +	 * In case RTE_CRYPTO_ASYM_FLAG_PLAIN_INPUT flag is set in the
> op flags field,
> +	 * it is a message to be signed by the PMD.
> +	 * Otherwise, it is a message hash.
> +	 */
> 
Please find above my comments for this flag.

>  	rte_crypto_uint k;
>  	/**< The ECDSA per-message secret number, which is an integer
> @@ -652,52 +653,20 @@ struct rte_crypto_asym_xform {
>  	};
>  };
> 
> -/**
> - * SM2 operation params.
> - */
> -struct rte_crypto_sm2_op_param {
> +struct rte_crypto_sm2_signature {

Yeah, it will help picking params for the application easily.
Just a suggestion: could we retain _param suffix. Say rte_crypto_sm2_sign_param.

>  	enum rte_crypto_asym_op_type op_type;
>  	/**< Signature generation or verification. */

Now op_type can either be sign/verify here.
> -
> -	rte_crypto_uint pkey;
> -	/**< Private key for encryption or sign generation. */
> -
> -	struct rte_crypto_ec_point q;
> -	/**< Public key for decryption or verification. */
> -
>  	rte_crypto_param message;
>  	/**<
> -	 * Pointer to input data
> -	 * - to be encrypted for SM2 public encrypt.
> -	 * - to be signed for SM2 sign generation.
> -	 * - to be authenticated for SM2 sign verification.
> -	 *
> -	 * Pointer to output data
> -	 * - for SM2 private decrypt.
> -	 * In this case the underlying array should have been
> -	 * allocated with enough memory to hold plaintext output
> -	 * (at least encrypted text length). The message.length field
> -	 * will be overwritten by the PMD with the decrypted length.
> -	 */
> -
> -	rte_crypto_param cipher;
> -	/**<
> -	 * Pointer to input data
> -	 * - to be decrypted for SM2 private decrypt.
> -	 *
> -	 * Pointer to output data
> -	 * - for SM2 public encrypt.
> -	 * In this case the underlying array should have been allocated
> -	 * with enough memory to hold ciphertext output (at least X bytes
> -	 * for prime field curve of N bytes and for message M bytes,
> -	 * where X = (C1 || C2 || C3) and computed based on SM2 RFC as
> -	 * C1 (1 + N + N), C2 = M, C3 = N. The cipher.length field will
> -	 * be overwritten by the PMD with the encrypted length.
> +	 * Pointer to the input data
> +	 * In case RTE_CRYPTO_ASYM_FLAG_PLAIN_INPUT flag is set in the
> op flags field,
> +	 * it is a message to be signed by the PMD.
> +	 * Otherwise, it is a message hash.
>  	 */
Please find above my comments for this flag.

> -
>  	rte_crypto_uint id;
> -	/**< The SM2 id used by signer and verifier. */
> -
> +	/**< The SM2 id used by signer and verifier.
> +	 * In case RTE_CRYPTO_ASYM_FLAG_PLAIN_INPUT flag is set this
> field is unused.
> +	 */
>  	rte_crypto_uint k;
>  	/**< The SM2 per-message secret number, which is an integer
>  	 * in the interval (1, n-1).
> @@ -719,6 +688,46 @@ struct rte_crypto_sm2_op_param {
>  	 */
>  };
> 
> +struct rte_crypto_sm2_cipher {
> +	enum rte_crypto_asym_op_type op_type;
> +	/**< Ecryption or decryption. */
> +	rte_crypto_param message;
> +	/**<
> +	 * Pointer to input data
> +	 * - to be encrypted for SM2 public encrypt.	 *
> +	 * Pointer to output data
> +	 * - for SM2 private decrypt.
> +	 */
> +	rte_crypto_param cipher;
> +	/**<
> +	 * Pointer to input data
> +	 * - to be decrypted for SM2 private decrypt.
> +	 *
> +	 * Pointer to output data
> +	 * - for SM2 public encrypt.
> +	 */
> +	rte_crypto_uint k;
> +	/**< The SM2 per-message secret number, which is an integer
> +	 * in the interval (1, n-1).
> +	 * If the random number is generated by the PMD,
> +	 * the 'rte_crypto_param.data' parameter should be set to NULL.
> +	 */
> +};
> +
> +/*
> + * Asymmetric SM2 transform data.
> + *
> + * Structure describing SM2 xform params.
> + */
> +struct rte_crypto_sm2_xform {
> +	enum rte_crypto_auth_algorithm hash;
> +	/**< Hash algorithm used in SM2 op. */
> +	rte_crypto_uint dA;
> +	/**< Private key. */
> +	struct rte_crypto_ec_point PA;
> +	/**< Public key. */
> +};
> +

sm2_xform is no more required, but ec_xform can be reused as I had proposed in:
[v1,4/6] cryptodev: use generic EC xform params for SM2

So, to summarize, may I request below of this patch to go above my patch series
If you agree.

(1). Splitting sm2 op params into sign and cipher ops.
(2). Move private and public keys from op param into EC xform.
       For DSA , should we move public key into xform as a session param ?

Thanks,
Gowrishankar

>  /**
>   * Asymmetric Cryptographic Operation.
>   *
> @@ -743,7 +752,8 @@ struct rte_crypto_asym_op {
>  		struct rte_crypto_dsa_op_param dsa;
>  		struct rte_crypto_ecdsa_op_param ecdsa;
>  		struct rte_crypto_ecpm_op_param ecpm;
> -		struct rte_crypto_sm2_op_param sm2;
> +		struct rte_crypto_sm2_signature sm2_signature;
> +		struct rte_crypto_sm2_cipher sm2_cipher;
>  	};
>  	uint16_t flags;
>  	/**<
> --
> 2.34.1