Re: [dpdk-dev] [RFC v1 1/1] lib/cryptodev: add support of asymmetric crypto

["Trahe, Fiona" <fiona.trahe@intel.com>]

Hi Shally,

> -----Original Message-----
> From: Verma, Shally [mailto:Shally.Verma@cavium.com]
> Sent: Wednesday, March 7, 2018 12:16 PM
> To: Trahe, Fiona <fiona.trahe@intel.com>; dev@dpdk.org; Athreya, Narayana Prasad
> <NarayanaPrasad.Athreya@cavium.com>; Murthy, Nidadavolu <Nidadavolu.Murthy@cavium.com>; Sahu,
> Sunila <Sunila.Sahu@cavium.com>; Gupta, Ashish <Ashish.Gupta@cavium.com>; Doherty, Declan
> <declan.doherty@intel.com>; Keating, Brian A <brian.a.keating@intel.com>; Griffin, John
> <john.griffin@intel.com>
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Subject: RE: [dpdk-dev] [RFC v1 1/1] lib/cryptodev: add support of asymmetric crypto
> 
> Hi Fiona
> 
> 
> >-----Original Message-----
> >From: Trahe, Fiona [mailto:fiona.trahe@intel.com]
> >Sent: 09 February 2018 23:43
> >To: dev@dpdk.org; Athreya, Narayana Prasad <NarayanaPrasad.Athreya@cavium.com>; Murthy,
> Nidadavolu
> ><Nidadavolu.Murthy@cavium.com>; Sahu, Sunila <Sunila.Sahu@cavium.com>; Gupta, Ashish
> <Ashish.Gupta@cavium.com>; Verma,
> >Shally <Shally.Verma@cavium.com>; Doherty, Declan <declan.doherty@intel.com>; Keating, Brian A
> <brian.a.keating@intel.com>;
> >Griffin, John <john.griffin@intel.com>
> >Cc: Trahe, Fiona <fiona.trahe@intel.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> >Subject: RE: [dpdk-dev] [RFC v1 1/1] lib/cryptodev: add support of asymmetric crypto
> >
> >Hi Shally,
> >Comments below.
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Shally Verma
> >> Sent: Tuesday, January 23, 2018 9:54 AM
> >> To: Doherty, Declan <declan.doherty@intel.com>
> >> Cc: dev@dpdk.org; pathreya@caviumnetworks.com; nmurthy@caviumnetworks.com;
> >> ssahu@caviumnetworks.com; agupta@caviumnetworks.com; Shally Verma
> >> <sverma@caviumnetworks.com>
> >> Subject: [dpdk-dev] [RFC v1 1/1] lib/cryptodev: add support of asymmetric crypto
> >>
> 
> //snip
> 
> >
> >> +	RTE_CRYPTO_ASYM_XFORM_FECC,
> >> +	/**< Fundamental Elliptic curve operations.
> >> +	 * Perform elliptic curve operation:
> >> +	 * add, double point, multiplication
> >> +	 * Refer to enum rte_crypto_fecc_optype
> >> +	 */
> >> +	RTE_CRYPTO_ASYM_XFORM_MODINV,
> >> +	/**< Modular Inverse */
> >[Fiona] would be nicer to group modinv with modexp
> 
> [Shally] I thought of it but having a xform RTE_CRYPTO_XFORM_MOD with two ops
> RTE_CRYPTO_OP_MOD_EXP and MOD_INV
> doesn’t seem to provide any benefit. Or do you have something else in mind?
[Fiona] I only meant to list them beside each other in the enum.  

> In addition, I am thinking probably we don’t need sessions for modexp or modinv ops. I am thinking to
> change their support as sessionless only.
> App can directly attach xform to compute modexp or inverse to op. What do you suggest?
[Fiona] That's ok for me.

> >> +	RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> >> +	/**< End of list */
> >> +};
> >> +
> >> +/**
> >> + * Asymmetric cryptogr operation type variants
> >[Fiona] typo: Use crypto or cryptographic
> >
> >> + */
> >> +enum rte_crypto_asym_op_type {
> >> +	RTE_CRYPTO_ASYM_OP_NOT_SPECIFIED = 1,
> >[Fiona] Why does this start at 1?
> >And is it necessary?
> >
> [Shally]  We need to indicate list of supported op in xform capability structure. Because an implementation
> may support RSA encrypt and decrypt but not RSA Sign and verify. Or, Can support DSA Sign compute but
> not verify.
> So, it was added to indicate end-of-array marker (though doesn’t need to be 1 for that reason). but now
> when I think to re-design its support, then it won't be needed.
> So, I thought rather than carrying op_type array, I can add an op_type bitmask in xform capability to show
> supported ops.
[Fiona] I think a bitmask is ok. Would only need an array if there were other params whose capabilities would
 vary depending on the op_type. E.g. like range of digest_size in sym depends on the algo. But does code below
not need a xform_type input? Or is capa the capability for one specific xform_type?

> Example capability check code then would look like:
> int rte_crypto_asym_check_op_type ( const rte_crypto_asym_capabilties *capa, int op_type) {
> 	If(capa->op_types & (1 << op_type))
> 		return 0;
> 	return -1;
> }
> 
> Please let me know your feedback, if you have any preferences here.
> 
> >> +	/**< Operation unspecified */
> >> +	RTE_CRYPTO_ASYM_OP_ENCRYPT,
> >> +	/**< Asymmetric encrypt operation */
> >> +	RTE_CRYPTO_ASYM_OP_DECRYPT,
> >> +	/**< Asymmetric Decrypt operation */
> >> +	RTE_CRYPTO_ASYM_OP_SIGN,
> >> +	/**< Signature generation operation */
> >> +	RTE_CRYPTO_ASYM_OP_VERIFY,
> >> +	/**< Signature verification operation */
> >> +	RTE_CRYPTO_ASYM_OP_KEY_PAIR_GENERATION,
> >> +	/**< Public/Private key pair generation operation */
> >[Fiona] In the comment, clarify that this is for DH and ECDH, and for the
> > generation of the public key (and optionally the private key?)
> >
> 
> [Shally] so far, I was assuming it will generate both but when you say private key optional, where you
> expect it to be coming from? - from app or generated internally? Is their hw variant which may not
> generate private key?
> 
[Fiona] In our native driver the private key, which is usually just a random number conforming to 
0 < private_key < (primeP - 1), is passed in by the application and only the public key is generated.
Some hw accelerators may have RNG capabilities, others may not or the application may prefer to generate
its own RN.


> //snip
> 
> >> +/**
> >> + * Fundamental ECC operation type variants.
> >> + */
> >> +enum rte_crypto_fecc_optype {
> >> +	RTE_CRYPTO_FECC_OP_NOT_SPECIFIED = 1,
> >> +	/**< FECC operation type unspecified */
> >[Fiona] as above. Why 1? And is it needed?
> 
> [Shally] This is for same reason to indicate in fecc xform capability list of supported op type in fundamental
> EC operation. And if we agree to use proposal above to use bitmask, it won't be needed.
[Fiona] ok

> >
> >> +	RTE_CRYPTO_FECC_OP_POINT_ADD,
> >> +	/**< Fundamental ECC point addition operation */
> >> +	RTE_CRYPTO_FECC_OP_POINT_DBL,
> >> +	/**< Fundamental ECC point doubling operation */
> >> +	RTE_CRYPTO_FECC_OP_POINT_MULTIPLY,
> >> +	/**< Fundamental ECC point multiplication operation */
> >> +	RTE_CRYPTO_FECC_OP_LIST_END
> >> +};
> >> +
> >> +#define RTE_CRYPTO_EC_CURVE_NOT_SPECIFIED  -1
> >[Fiona] Wouldn't it be better to put this back in as the initial value in the enum as originally done?
> >Else will there not be a compiler warning if a param of that enum type is
> >initialised to above #define?
> >And are _BINARY and _PRIME values needed in this case?
> 
> [Shally] Agreed. We would need to use typecast to avoid warning so I will revert and define _Primary and
> Binary variant. But before that I have one question on published list. See below.
> 
> >
> >> +/**
> >> + * ECC list of curves.
> >> + */
> //snip
> 
> >> +	/**< NIST/SECG/WTLS curve over a 163 bit binary field */
> >> +	RTE_CRYPTO_EC_CURVE_wap_wsg_idm_ecid_wtls4,
> >> +	/**< SECG curve over a 113 bit binary field */
> >> +	RTE_CRYPTO_EC_CURVE_wap_wsg_idm_ecid_wtls5,
> >> +	/**< X9.62 curve over a 163 bit binary field */
> >> +	RTE_CRYPTO_EC_CURVE_wap_wsg_idm_ecid_wtls10,
> >> +	/**< NIST/SECG/WTLS curve over a 233 bit binary field */
> >> +	RTE_CRYPTO_EC_CURVE_wap_wsg_idm_ecid_wtls11,
> >> +	/**< NIST/SECG/WTLS curve over a 233 bit binary field */
> >> +	RTE_CRYPTO_EC_BCURVE_LIST_END
> >> +};
> >[Fiona] Do we really want to specify curves < 224 bits? Even 224 is woefully insecure these days.
> >Also do we want to list all "published" curves, or allow customers to specify
> >their own curve parameters, at least on the FECC API? Adding an API to specify a curve with
> >its params would allow new curves to be used without affecting the API and
> >would avoid the need for such an extensive list.
> 
> [Shally] I would take this question in-parts:
> 
> " Also do we want to list all "published" curves, or allow customers to specify their own curve parameters,"
> - Currently specification allow app to do both i.e. it can either setup these published curve ids or specify its
> own domain params to all elliptic curve based xforms (ecdh, ecdsa, and fecc).
>   If input curve has curve_type = UNSPECIFIED, PMD uses domain parameters else it uses curveid given by
> app from this published list.
>   So, is this missing any requirement that need to be supported?!
[Fiona] So you mean the params in ECDH xform (a,b,G,n,h) are only specified if curve_type = UNSPECIFIED,
else not needed? And in FECC the params (order,G,a,b,h) ?
Ecdsa xform not yet specified, but it will have a similar set?
Then I would suggest creating a struct to hold this param set and using same struct in all 3 xforms.

> 
> " Adding an API to specify a curve with its params would allow new curves to be used without affecting the
> API and would avoid the need for such an extensive list."
> - help me understand a bit here. Are you suggesting, we can remove this publish list and add a new API
> such as
> 
> int rte_crypto_asym_new_curve(int curveid, rte_crypto_asym_ec_domain_params params)
> 
> which app can call to create its curveid with its own domain parameters in PMD?
[Fiona] Yes, I was thinking of something like that. But above approach to pass in on the xforms is ok
and probably easier to manage.
> 
> If yes, then how it would help remove published list? Does that mean that we assume app to retrieve
> domain params from these standard and re-create again on PMD?
> 
> "do we have to publish curves < 224 bits"
> - We just listed all based on previous feedback to include non-nist curve id. But agree it can be narrowed
> down to few (may be to one used by ssl)
[Fiona] I'm looking for feedback internally on this - will get back to you later.
But I think it could be trimmed to at least removing those curves < 224 bits.
The xform params can be used to cover unlikely curves trimmed from the list.
 
> >Do we need a "capabilities" mask to say which curves are supported by a device?
> 
> [Shally] Yes, if we are supporting published id.
[Fiona] ok
 
> >What about SM2?
> [Shally] app can setup ec params as mentioned in sm2 spec. does it need any explicit support on API?
[Fiona] ok

> 
> >
> //snip
> 
> >> +
> >> +/**
> >> + * Elliptic curve id
> >> + */
> >> +struct rte_crypto_ec_curve_id {
> >> +	RTE_STD_C11
> >> +	union {
> >> +		enum rte_crypto_ec_prime_curve pcurve;
> >> +		enum rte_crypto_ec_binary_curve bcurve;
> >> +	};
> >> +};
> >[Fiona] Because the values of these two enums overlap, if you specify the curve type incorrectly, you'll still
> >have a potentially valid curve enum specified. I suggest it's safer to include the type here with the union,
> >rather than in the wrapper xform struct, i.e.
> >struct rte_crypto_ec_curve {
> >	enum rte_crypto_ec_curve_type curve_type;
> >	/**< curve type: Prime vs Binary */
> >	union {
> >	enum rte_crypto_ec_prime_curve pcurve_id;
> >	enum rte_crypto_ec_binary_curve bcurve_id;
> >	};
> >};
> [Shally] We would need curve type if we need to define a new curve based on domain params. Let's cover
> it later once we clarify above feedback.
[Fiona] So how about adding the struct with curve params I mentioned above as a 3rd element in the union?

> 
> >> +/**
> >> + * Asymmetric RSA transform data
> >> + *
> >> + * This structure contains data required to perform RSA crypto
> >> + * transform.
> >> + *
> >> + */
> >> +struct rte_crypto_rsa_xform {
> >> +
> >> +	rte_crypto_xform_param n;
> >> +	/**< n - Prime modulus
> >> +	 * Prime modulus data of RSA operation in Octet-string network
> >> +	 * byte order format.
> >> +	 */
> >> +
> >> +	rte_crypto_xform_param e;
> >> +	/**< e - Public key exponent
> >> +	 * Public key exponent used for RSA public key operations in Octet-
> >> +	 * string network byte order format.
> >> +	 */
> >> +
> >> +	rte_crypto_xform_param d;
> >> +	/**< d - Private key exponent
> >> +	 * Private key exponent used for RSA private key operations in
> >> +	 * Octet-string  network byte order format.
> >> +	 */
> >> +
> >> +	rte_crypto_xform_param p;
> >> +	/**< p - Private key component P
> >> +	 * Private key component of RSA parameter  required for CRT method
> >> +	 * of private key operations in Octet-string network byte order
> >> +	 * format.
> >> +	 */
> >> +
> >> +	rte_crypto_xform_param q;
> >> +	/**< q - Private key component Q
> >> +	 * Private key component of RSA parameter  required for CRT method
> >> +	 * of private key operations in Octet-string network byte order
> >> +	 * format.
> >> +	 */
> >> +
> >> +	rte_crypto_xform_param dP;
> >> +	/**< dP - Private CRT component
> >> +	 * Private CRT component of RSA parameter  required for CRT method
> >> +	 * RSA private key operations in Octet-string network byte order
> >> +	 * format.
> >> +	 * dP = d mod ( p - 1 )
> >> +	 */
> >> +
> >> +	rte_crypto_xform_param dQ;
> >> +	/**< dQ - Private CRT component
> >> +	 * Private CRT component of RSA parameter  required for CRT method
> >> +	 * RSA private key operations in Octet-string network byte order
> >> +	 * format.
> >> +	 * dQ = d mod ( q - 1 )
> >> +	 */
> >> +
> >> +	rte_crypto_xform_param qInv;
> >> +	/**< qInv - Private CRT component
> >> +	 * Private CRT component of RSA parameter  required for CRT method
> >> +	 * RSA private key operations in Octet-string network byte order
> >> +	 * format.
> >> +	 * qInv = inv q mod p
> >> +	 */
> >> +};
> >[Fiona] This allows both representations of the private key to be specified
> >with the possibility of confusion if both are specified.
> >Would it be better to have a union of 2 different structures, one with {n,d} and
> >the other with {p,q,dP,dQ,qInv}?
> >
> 
> [Shally] Ok.
> 
> //snip
> 
> >> +
> >> +/**
> >> + * Asymmetric crypto transform data
> >> + *
> >> + * This structure contains the data required to perform the
> >> + * asymmetric crypto transformation operation. The field op
> >> + * determines the asymmetric algorithm for transformation.
> >[Fiona] clarify comment. The xform only contains some of the data, the rest will be provided in the op.
> >And there's no field op here.
> 
> [Shally] Ok
> 
> >> + */
> >> +struct rte_crypto_asym_xform {
> >> +	struct rte_crypto_asym_xform *next;
> >[Fiona] Is there any reason for a chain of xforms?
> >I'd suggest removing unless needed.
> >
> 
> [Shally] Yes. One possible chain ECDH key generation followed by ECDSA sign compute. Similar with DH key
> generation + DSA sign compute.
> Currently these are only valid chain I foresee that can be setup to be performed on an op using that
> session. Can add other if we identify more such dependent
> set of operation.
[Fiona] ok

> 
> >> +	enum rte_crypto_asym_xform_type xform_type;
> >> +	/**< Asymmetric algorithm for crypto transform */
> >> +
> >> +	RTE_STD_C11
> >> +	union {
> >> +		struct rte_crypto_rsa_xform rsa;
> >> +		/**< RSA xform parameters */
> >> +
> >> +		struct rte_crypto_fecc_xform fecc;
> >> +		/**< Fundamental Elliptic Curve xform parameters */
> >> +
> >> +		struct rte_crypto_modex_xform modex;
> >> +		/**< Modular Exponentiation xform parameters */
> >> +
> >> +		struct rte_crypto_modinv_xform modinv;
> >> +		/**< Modulus Inverse xform parameters */
> >> +
> >> +		struct rte_crypto_ecdsa_xform ecdsa;
> >> +		/**< ECDSA xform parameters */
> >> +
> >> +		struct rte_crypto_ecdh_xform ecdh;
> >> +		/**< ECDH xform parameters */
> >> +
> >> +		struct rte_crypto_dsa_xform dsa;
> >> +		/**< DSA xform parameters */
> >> +	};
> >> +};
> 
> //snip
> 
> >>  #define RTE_CRYPTODEV_DETACHED  (0)
> >> @@ -513,6 +677,24 @@ struct rte_cryptodev_config {
> >>  	int socket_id;			/**< Socket to allocate resources on */
> >>  	uint16_t nb_queue_pairs;
> >>  	/**< Number of queue pairs to configure on device */
> >> +
> >> +	uint16_t nb_symm_qp;
> >> +	/**< Number of queue pair to be used for symmetric operations.
> >> +	 * Optional input.
> >> +	 * Valid for device supporting both symmetric and asymmetric.
> >> +	 * Should be less than equal to rte_cryptodev_info:max_nb_symm_qp.
> >> +	 * please note this number only tells how many queue pair to be used
> >> +	 * for symmetric op and does *not* tell specific IDs to be used.
> >> +	 */
> >> +
> >> +	uint16_t nb_asymm_qp;
> >> +	/**< Number of queue pair to be used for asymmetric operations.
> >> +	 * Optional input.
> >> +	 * Valid for device supporting both asymmetric and symmetric.
> >> +	 * Should be less than equal to rte_cryptodev_info:max_nb_asymm_qp
> >> +	 * Please note this number only tells how many queue pair to be
> >> +	 * used for asymmetric and does *not* specifically tell qp ID
> >> +	 */
> >>  };
> >>
> >[Fiona] The params from config structure above should be used to set the values in the data structure
> >This code is missing.
> 
> [Shally] as discussed , such changes about sym/asym qp would be removed
> 
> >
> //snip
> 
> >> @@ -975,6 +1253,8 @@ struct rte_cryptodev_sym_session *
> >>
> >>  /**
> >>   * Get the size of the private session data for a device.
> >> + * if device support both symmetric and asymmetric, it
> >> + * return size inclusive of both
> >[Fiona] Are you suggesting the same session pool might be used
> >for both sym and asym sessions?
> >Seems unlikely, but if an application chose to, it seems better
> > that it would do it explicitly,
> >e.g. get asym and sym private sizes, same as it gets size from the other
> >drivers and sizes for the maximum.
> >If meaning is changed as you suggest then sym applications already
> >implemented will likely end up with pools sized for a session size they don’t need.
> >
> >As I think the combined pool case unlikely, it makes more sense to me that this
> >fn should have comment changed to say it returns only sym session size
> >for backwards capability reasons, but will be deprecated
> >and appls should use get_sym_session_private_size instead.
> 
> [Shally] Ok agree. I will revert to its original definition.
> >
> 
> // snip
> 
> //snip
> 
> >> +/**
> >> + * Create a asymmetric session mempool to allocate sessions from
> >> + *
> >> + * @param	dev		Crypto device pointer
> >> + * @param	nb_objs		Number of asymmetric sessions objects in mempool
> >> + * @param	obj_cache	l-core object cache size, see *rte_ring_create*
> >> + * @param	socket_id	Socket Id to allocate  mempool on.
> >> + *
> >> + * @return
> >> + * - On success returns a pointer to a rte_mempool
> >> + * - On failure returns a NULL pointer
> >> + */
> >> +typedef int (*cryptodev_asym_create_session_pool_t)(
> >> +		struct rte_cryptodev *dev, unsigned nb_objs,
> >> +		unsigned obj_cache_size, int socket_id);
> >>
> >[Fiona] session pools are not created now per device so this is never called for sym and
> >should be deprecated. So no need to add for asym
> 
> [Shally] Ok
> 
> >
> //snip
> 
> >> +	cryptodev_sym_get_session_private_size_t sym_session_get_size;
> >> +	/**< Return private size for symmetric crypto. */
> >> +	cryptodev_sym_get_session_private_size_t asym_session_get_size;
> >> +	/**< Return private size for asymmetric crypto. */
> >>  	cryptodev_sym_configure_session_t session_configure;
> >>  	/**< Configure a Crypto session. */
> >[Fiona] This should really be renamed sym_session_configure, same for session_clear,
> >qp_attach_session and qp_detach_session
> >
> 
> [Shally] I thought to keep it backward compatible for now. I think we can take this change later as it need
> announcement first
[Fiona] As these are on the internal API between PMDs and API layer, rather than the application<->cryptodev
interface I think they don't need an announcement.
But would need to be done in a standalone patch, with all PMDs changed

> >> +	cryptodev_asym_configure_session_t asym_session_configure;
> >> +	/**< Configure asymmetric Crypto session. */
> 
> //snip
> 
> >> +
> >> +EXPERIMENTAL {
> >> +	global:
> >> +
> >[Fiona] the implementation of all these APIs also needs the _experimental tag.
> >
> 
> [Shally] Got that.
> 
> >> +	rte_cryptodev_asym_capability_get;
> >> +	rte_cryptodev_asym_capability_modlen;
> >> +	rte_cryptodev_asym_queue_pair_count;
> >> +	rte_cryptodev_asym_session_create;
> >> +	rte_cryptodev_asym_session_clear;
> >> +	rte_cryptodev_asym_session_free;
> >> +	rte_cryptodev_asym_session_init;
> >> +	rte_cryptodev_get_asym_session_private_size;
> >> +	rte_cryptodev_get_sym_session_private_size;
> >> +	rte_cryptodev_get_xform_algo_enum;
> >> +	rte_cryptodev_queue_pair_attach_asym_session;
> >> +	rte_cryptodev_queue_pair_detach_asym_session;
> >> +	rte_cryptodev_sym_queue_pair_count;
> >> +	rte_crypto_asym_xform_strings;
> >> +	rte_crypto_asym_operation_strings;
> >> +
> >> +};
> >> --
> >> 1.9.1
> 
> Thanks
> Shally