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

[Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>]

> Acked-by: Arek Kusztal <arkadiuszx.kusztal@intel.com>
> 
> Some things will need to be changed before the next release (few of them I
> described below), but I will ack it. Especially that other algorithms have similar
> issues.
> 
Sure. I also thought any improvements can collectively addressed as it is another EC.

> > +/**
> > + * 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 @@ -637,9 +653,79 @@
> > 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 {
> > +	enum rte_crypto_asym_op_type op_type;
> > +	/**< Signature generation or verification */
> > +
> > +	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.
> > +	 *
> 
> This repeats problems known to dsa/ecdsa. What will work on OpenSSL PMD will
> not work on the HW. Ironically, test will pass for both...
> We can extend this before the next release.

Ack
> 
> > +	 * 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.
> > +	 */
> I thought it was concatenation, not addition.

Typo I did. Yes it should have been ||.

> > +
> > +	rte_crypto_uint id;
> > +	/**< The SM2 id used by signer and verifier and is in interval (1,
> > +n-1). */
> Where does the (1,n-1) limitation comes from? As it is a hashed prefix, should it
> have any mathematical interpretation at all?

Yeah it is not necessarily limited wrt n. we can just phrase it:
/**< The SM2 id used by signer and verifier */

In fact, here could be another improvement (applicable to EC as well) to 
keep the params specific to op. Sign/verify only would need this param
(and r , s below).

> > +
> > +	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.
> > +	 */
> > +
> > +	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
> > +	 */
> > +};
> > +