RE: [PATCH v2 1/3] cryptodev: add SM2 asymmetric crypto algorithm

[Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>]

Hi ArkadiuszX,

> > ShangMi 2 (SM2) is a encryption and digital signature algorithm used
> > in the Chinese National Standard.
> 
> It is more of a set of public-key cryptography algorithms based on elliptic curves.
Ok I ll rephrase it.

> 
> >
...
> > ---
> >  doc/guides/cryptodevs/features/default.ini |  1 +
> >  doc/guides/rel_notes/release_23_07.rst     |  5 ++
> >  lib/cryptodev/rte_crypto_asym.h            | 77 ++++++++++++++++++++++
> >  lib/cryptodev/rte_cryptodev.c              |  1 +
> >  4 files changed, 84 insertions(+)
> >
> > diff --git a/doc/guides/cryptodevs/features/default.ini
> > b/doc/guides/cryptodevs/features/default.ini
> > index 523da0cfa8..a69967bb9e 100644
> > --- a/doc/guides/cryptodevs/features/default.ini
> > +++ b/doc/guides/cryptodevs/features/default.ini
> > @@ -125,6 +125,7 @@ Diffie-hellman          =
> >  ECDSA                   =
> >  ECPM                    =
> >  ECDH                    =
> > +SM2                     =
> >
> >  ;
> >  ; Supported Operating systems of a default crypto driver.
> > diff --git a/doc/guides/rel_notes/release_23_07.rst
> > b/doc/guides/rel_notes/release_23_07.rst
> > index a9b1293689..8b8e69d619 100644
> > --- a/doc/guides/rel_notes/release_23_07.rst
> > +++ b/doc/guides/rel_notes/release_23_07.rst
> > @@ -55,6 +55,11 @@ New Features
> >       Also, make sure to start the actual text at the margin.
> >       =======================================================
> >
> > +* **Added SM2 asymmetric algorithm in cryptodev.**
> > +
> > +  Added support for ShamMi 2 (SM2) asymmetric crypto algorithm  along
> > + with prime field curve support.
> > +
> >
> >  Removed Items
> >  -------------
> > diff --git a/lib/cryptodev/rte_crypto_asym.h
> > b/lib/cryptodev/rte_crypto_asym.h index 989f38323f..35fa2c0a6d 100644
> > --- a/lib/cryptodev/rte_crypto_asym.h
> > +++ b/lib/cryptodev/rte_crypto_asym.h
> > @@ -119,6 +119,11 @@ enum rte_crypto_asym_xform_type {
> >  	/**< Elliptic Curve Point Multiplication */
> >  	RTE_CRYPTO_ASYM_XFORM_ECFPM,
> >  	/**< Elliptic Curve Fixed Point Multiplication */
> > +	RTE_CRYPTO_ASYM_XFORM_SM2,
> > +	/**< ShangMi 2
> > +	 * Performs Encrypt, Decrypt, Sign and Verify.
> > +	 * Refer to rte_crypto_asym_op_type.
> > +	 */
> >  	RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> >  	/**< End of list */
> >  };
> > @@ -382,6 +387,20 @@ struct rte_crypto_ec_xform {
> >  	/**< Pre-defined ec groups */
> >  };
> >
> > +/**
> > + * Asymmetric SM2 transform data
> > + *
> > + * Structure describing SM2 xform params
> > + *
> > + */
> > +struct rte_crypto_sm2_xform {
> > +	rte_crypto_uint pkey;
> > +	/**< Private key of the signer for signature generation. */
> > +
> > +	struct rte_crypto_ec_point q;
> > +	/**< Public key of the signer for verification. */ };
> > +
> >  /**
> >   * Operations params for modular operations:
> >   * exponentiation and multiplicative inverse @@ -637,9 +656,66 @@
> > struct rte_crypto_asym_xform {
> >  		/**< EC xform parameters, used by elliptic curve based
> >  		 * operations.
> >  		 */
> > +
> > +		struct rte_crypto_sm2_xform sm2;
> > +		/**< SM2 xform parameters */
> >  	};
> >  };
> >
> > +/**
> > + * SM2 operation params
> > + */
> > +struct rte_crypto_sm2_op_param {
> 
> There is no random value 'k'. And SM2 is also using it for encryption.

Agreed. But if we see openSSL kind of SW library, it does not take this param from app. 
(for eg sign/encrypt), instead it generates on its own (still through RNG as I see).
so, shall we support either way meaning extern_scalar or inbuilt_scalar ? I am thinking
we can add enum like below:
- RTE_EC_SCALAR_INBUILT 
- RTE_EC_SCALAR_EXTERNAL

With inbuilt scalar, this k value in param can be ignored by PMD, while in external case,
pass it to HW.

> There is no key exchange or point multiplication option in this op, therefore, I
> would rather have all SM2 algorithms in separate ops.
You mean, we shall have structs rte_crypto_sm2_sign_op, rte_crypto_sm2_verify_op etc 
dedicated for each op ?

> We also could abandon finally '_param' suffix, it does not add clarity but extends
> struct tags, which are very long already.
Only for SM2 or for all algorithms ?

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

I need to add correction here. Op type can also be encrypt or decrypt.
> > +
> > +	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.
> 
> Is 'message' in signature case here plaintext or hash?
As per RFC draft, it seems plain text, because we add prefix to it and then H256().

> If 'plaintext' it is inconsistent with other signature algorithms, and most likely
> not all HW devices will support that.
It is just another EC right ? Even in EC op param, we don't distinguish plaintext/hash message.

> Additionally or hash function should be specified as an argument by the user, or
> the function used should be defined in the API information. I see it is not directly
> said in this draft what function it ought be, although most likely
> SM3 would be the one.

Agree, SM2 sign implementation would need to know which hash algo to pick.
Hence, we can add hash function as param. RFC does not list options but quotes that
Algorithm should have been approved by Chinese Commercial Cryptography Administration Office.
So I am not too sure if we could add SHA3/SHAKE apart from SHA256 if it is approved.
 - SM3
 - SHA256
 - SHA3_256
 - SHAKE_256

May be PMD could evaluate it and return -ENOTSUP accordingly ?
> 
> > +	 *
> > +	 * 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.
> > +	 */
> > +
> > +	rte_crypto_uint id;
> > +	/**< The SM2 id used by signer and verifier and is in interval (1,
> > +n-1). */
> 
> What is this parameter exactly, is it an ID prepended to the signature input?
> 
This is signer identifier as per RFC. This gets concatenated with other EC params:
  ZA=H256(ENTLA || IDA || a || b || xG || yG || xA || yA)

ZA is then once again added with message before one more hash on entire thing.
> > +
> > +	rte_crypto_uint r;
> > +	/**< r component of elliptic curve signature
> > +	 *     output : for signature generation (of at least N bytes
> > +	 *              where prime field length is N bytes)
> > +	 *     input  : for signature verification
> > +	 */
> > +	rte_crypto_uint s;
> > +	/**< s component of elliptic curve signature
> > +	 *     output : for signature generation (of at least N bytes
> > +	 *              where prime field length is N bytes)
> > +	 *     input  : for signature verification
> > +	 */
> > +};
> > +
> >  /**
> >   * Asymmetric Cryptographic Operation.
> >   *
> > @@ -665,6 +741,7 @@ 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;
> >  	};
> >  	uint16_t flags;
> >  	/**<
> > diff --git a/lib/cryptodev/rte_cryptodev.c
> > b/lib/cryptodev/rte_cryptodev.c index a96114b2da..21cabc3ffd 100644
> > --- a/lib/cryptodev/rte_cryptodev.c
> > +++ b/lib/cryptodev/rte_cryptodev.c
> > @@ -299,6 +299,7 @@ crypto_asym_xform_strings[] = {
> >  	[RTE_CRYPTO_ASYM_XFORM_DSA]	= "dsa",
> >  	[RTE_CRYPTO_ASYM_XFORM_ECDSA]	= "ecdsa",
> >  	[RTE_CRYPTO_ASYM_XFORM_ECPM]	= "ecpm",
> > +	[RTE_CRYPTO_ASYM_XFORM_SM2]	= "sm2",
> >  };
> >
> >  /**
> > --
> > 2.25.1

Thanks for the review,
Gowrishankar