Re: [dpdk-dev] [PATCH 1/4] lib/crypto: add support for ECDSA

[Anoob Joseph <anoobj@marvell.com>]

Hi Akhil,

Do you suggest any change for the title here? As in, should I make it,

cryptodev: support ECDSA

@Arek, please see inline for responses.

Thanks,
Anoob

> -----Original Message-----
> From: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>
> Sent: Thursday, January 9, 2020 6:34 PM
> To: Anoob Joseph <anoobj@marvell.com>; Akhil Goyal <akhil.goyal@nxp.com>;
> Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>
> Cc: Ayuj Verma <ayverma@marvell.com>; Trahe, Fiona
> <fiona.trahe@intel.com>; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> Narayana Prasad Raju Athreya <pathreya@marvell.com>; Shally Verma
> <shallyv@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>; Sunila Sahu
> <ssahu@marvell.com>; dev@dpdk.org
> Subject: [EXT] RE: [PATCH 1/4] lib/crypto: add support for ECDSA
> 
> External Email
> 
> ----------------------------------------------------------------------
> Hi Anoob,
> 
> > -----Original Message-----
> > From: Anoob Joseph <anoobj@marvell.com>
> > Sent: Thursday, January 2, 2020 8:55 AM
> > To: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>; Akhil Goyal
> > <akhil.goyal@nxp.com>; Doherty, Declan <declan.doherty@intel.com>; De
> > Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> > Cc: Ayuj Verma <ayverma@marvell.com>; Trahe, Fiona
> > <fiona.trahe@intel.com>; Jerin Jacob Kollanukkaran
> > <jerinj@marvell.com>; Narayana Prasad Raju Athreya
> > <pathreya@marvell.com>; Shally Verma <shallyv@marvell.com>; Ankur
> > Dwivedi <adwivedi@marvell.com>; Sunila Sahu <ssahu@marvell.com>;
> > dev@dpdk.org
> > Subject: RE: [PATCH 1/4] lib/crypto: add support for ECDSA
> >
> > Hi Arek,
> >
> > Please see inline.
> >
> > Thanks,
> > Anoob
> >
> > > -----Original Message-----
> > > From: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>
> > > Sent: Friday, December 20, 2019 9:36 PM
> > > To: Anoob Joseph <anoobj@marvell.com>; Akhil Goyal
> > > <akhil.goyal@nxp.com>; Doherty, Declan <declan.doherty@intel.com>;
> > > De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> > > Cc: Ayuj Verma <ayverma@marvell.com>; Trahe, Fiona
> > > <fiona.trahe@intel.com>; Jerin Jacob Kollanukkaran
> > > <jerinj@marvell.com>; Narayana Prasad Raju Athreya
> > > <pathreya@marvell.com>; Shally Verma <shallyv@marvell.com>; Ankur
> > > Dwivedi <adwivedi@marvell.com>; Sunila Sahu <ssahu@marvell.com>;
> > > dev@dpdk.org
> > > Subject: [EXT] RE: [PATCH 1/4] lib/crypto: add support for ECDSA
> > >
> > > External Email
> > >
> > > --------------------------------------------------------------------
> > > --
> > > Hi Anoob,
> > >
> > > Few suggestions inline.
> > >
> > > >  ;
> > > >  [Asymmetric]
> > > > -RSA =
> > > > -DSA =
> > > > -Modular Exponentiation =
> > > > -Modular Inversion =
> > > > -Diffie-hellman =
> > > > \ No newline at end of file
> > > > +RSA                     =
> > > > +DSA                     =
> > > > +Modular Exponentiation  =
> > > > +Modular Inversion       =
> > > > +Diffie-hellman          =
> > > > +ECDSA                   =
> > > > diff --git a/lib/librte_cryptodev/rte_crypto_asym.h
> > > > b/lib/librte_cryptodev/rte_crypto_asym.h
> > > > index 0d34ce8..dd5e6e3 100644
> > > > --- a/lib/librte_cryptodev/rte_crypto_asym.h
> > > > +++ b/lib/librte_cryptodev/rte_crypto_asym.h
> > > > @@ -81,6 +81,10 @@ enum rte_crypto_asym_xform_type {
> > > >  	/**< Modular Exponentiation
> > > >  	 * Perform Modular Exponentiation b^e mod n
> > > >  	 */
> > > > +	RTE_CRYPTO_ASYM_XFORM_ECDSA,
> > > > +	/**< Elliptic Curve Digital Signature Algorithm
> > > > +	 * Perform Signature Generation and Verification.
> > > > +	 */
> > > >  	RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> > > >  	/**< End of list */
> > > >  };
> > > > @@ -319,6 +323,46 @@ struct rte_crypto_dsa_xform {  };
> > > >
> > > >  /**
> > > > + * TLS named curves
> > > > + *
> > > > +https://urldefense.proofpoint.com/v2/url?u=https-
> > > 3A__www.iana.org_ass
> > > > +ignments_tls-
> > > 2Dparameters_&d=DwIFAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=jPfB8r
> > > > +wwviRSxyLWs2n6B-WYLn1v9SyTMrT5EQqh2TU&m=nYBsHHO1NEdu-
> > > JmNULDH0kx7auwQd
> > > >
> > >
> > +SbI0VYNVNxmm1w&s=RKZgomFiHpm9Yp8AYEn4FjlPpzbdavkMv5cRUggVid
> > > A&e=
> > > > + * tls-parameters.xhtml#tls-parameters-8
> > > > + * secp192r1 = 19,
> > > > + * secp224r1 = 21,
> > > > + * secp256r1 = 23,
> > > > + * secp384r1 = 24,
> > > > + * secp521r1 = 25,
> > > > + */
> > > > +enum rte_crypto_ec_group {
> > > > +	RTE_CRYPTO_EC_GROUP_UNKNOWN  = 0,
> > > > +	RTE_CRYPTO_EC_GROUP_NISTP192 = 19,
> > > > +	RTE_CRYPTO_EC_GROUP_NISTP224 = 21,
> > > > +	RTE_CRYPTO_EC_GROUP_NISTP256 = 23,
> > > > +	RTE_CRYPTO_EC_GROUP_NISTP384 = 24,
> > > > +	RTE_CRYPTO_EC_GROUP_NISTP521 = 25, };
> > >
> > > [Arek] Since in comment we use SECG naming maybe enum should follow
> > to
> > > avoid confusion?
> >
> > [Anoob] How about RTE_CRYPTO_EC_GROUP_SECP521R1 and so on?
> [Arek] Iam ok with that.

[Anoob] Will update in v2.
 
> >
> > > > +
> > > > +/**
> > > > + * Structure for elliptic curve point  */ struct
> > > > +rte_crypto_ec_point {
> > > > +	rte_crypto_param x;
> > > > +	/**< X coordinate */
> > > > +	rte_crypto_param y;
> > > > +	/**< Y coordinate */
> > > > +};
> > > > +
> > > > +/**
> > > > + * Asymmetric elliptic curve transform data
> > > > + *
> > > > + * Structure describing all EC based xform params
> > > > + *
> > > > + */
> > > > +struct rte_crypto_ec_xform {
> > > > +	enum rte_crypto_ec_group curve_id;
> > > > +	/**< Pre-defined ec groups */
> > > > +};
> > > > +
> > > > +/**
> > > >   * Operations params for modular operations:
> > > >   * exponentiation and multiplicative inverse
> > > >   *
> > > > @@ -372,6 +416,11 @@ struct rte_crypto_asym_xform {
> > > >
> > > >  		struct rte_crypto_dsa_xform dsa;
> > > >  		/**< DSA xform parameters */
> > > > +
> > > > +		struct rte_crypto_ec_xform ec;
> > > > +		/**< EC xform parameters, used by elliptic curve based
> > > > +		 * operations.
> > > > +		 */
> > > >  	};
> > > >  };
> > > >
> > > > @@ -516,6 +565,39 @@ struct rte_crypto_dsa_op_param {  };
> > > >
> > > >  /**
> > > > + * ECDSA operation params
> > > > + */
> > > > +struct rte_crypto_ecdsa_op_param {
> > > > +	enum rte_crypto_asym_op_type op_type;
> > > > +	/**< Signature generation or verification */
> > > > +
> > > > +	rte_crypto_param pkey;
> > > > +	/**< Private key of the signer for signature generation */
> > > [Arek] - for DSA we have private key in xform, why this inconsistency?
> >
> > [Anoob] Putting key in the session would limit the number of ops we
> > can perform with one session. In regular use cases, the number of ops
> > done with one key is very minimal. But the number of ops using same EC group
> is more.
> > So if we define the session to one per EC group, then we will have
> > better usage of session. Otherwise, the session need to be created and
> > destroyed every few operations.
> [Arek] I agree with that, so maybe we could change DSA similar way (leave only
> group params?)

[Anoob] I'm okay with that. But that will have to be taken up separately, I guess. Shall I take that you are in agreement with the proposed change here?
 
> >
> > > > +
> > > > +	struct rte_crypto_ec_point q;
> > > > +	/**< Public key of the signer for verification */
> > > > +
> > > > +	rte_crypto_param message;
> > > > +	/**< Input message to be signed or verified */
> > > [Arek] - This I expect should be message digest instead of message itself?
> >
> > [Anoob] Yes. Message digest is what is expected. This is following
> > what is done with DSA & RSA. Do you recommend any changes? Variable
> > name or description.
> [Arek] Some information would be good I think.

[Anoob] I'll update the comment to state the same

/**< Input message digest to be signed or verified */

Is this fine?
 
> >
> > > > +
> > > > +	rte_crypto_param k;
> > > > +	/**< The ECDSA per-message secret number, which is an integer
> > > > +	 * in the interval (1, n-1)
> > > > +	 */
> > > [Arek] - If pmd can generate 'k' internally we could do something like:
> > > 'if k.data == NULL => PMD will generate 'k' internally, k.data
> > > remains untouched.'
> >
> > [Anoob] So that will be exposed as a new capability, right? Do you
> > suggest any changes here to accommodate that?
> [Arek] Or maybe feature flag, it would apply to DSA as well.

[Anoob] I meant feature flag only! Capability is for net devs etc.
 
> >
> > > Another option is to provide user with some callback function to
> > > generate CSRN which could be useful for RSA PSS, OAEP as well (we
> > > already discussed that internally in Intel, I will elaborate on this
> > > bit more in
> > different thread).
> >
> > [Anoob] Do you intend to keep the generated CSRN hidden in the PMD?
> [Arek] Openssl PMD does that with DSA but iam not a fan of that. But if some hw
> can do DSA/ECDSA by generating 'k' by itself it would have to be added anyway.
> Other option is to add aforementioned callbacks (for HW that not generate 'k'
> by itself)

[Anoob] I think we can support either case. Our h/w also can generate CSRN and so are open to ideas. Can you explain the proposed callback function? Also, we can defer that discussion to a separate thread, if we have an agreement on the approach here. 
 
> >
> > >
> > > > +
> > > > +	rte_crypto_param r;
> > > > +	/**< r component of elliptic curve signature
> > > > +	 *     output : for signature generation
> > > > +	 *     input  : for signature verification
> > > > +	 */
> > > > +	rte_crypto_param s;
> > > > +	/**< s component of elliptic curve signature
> > > > +	 *     output : for signature generation
> > > > +	 *     input  : for signature verification
> > > > +	 */
> > > [Arek] - Do we want to add any constraints like 'this field should
> > > be big enough to hold...'
> >
> > [Anoob] For every case where rte_crypto_param is used for 'output',
> > application should make sure the buffers are large enough. Do you
> > think we could document it somewhere common instead of adding per
> operation?
> [Arek]
> Both options look good to me.

[Anoob] How about updating the description of 'rte_crypto_param' to reflect the same? I can update it in v2, if you can confirm.
 
> >
> > > > +};
> > > > +
> > > > +/**
> > > >   * Asymmetric Cryptographic Operation.
> > > >   *
> > > >   * Structure describing asymmetric crypto operation params.
> > > > @@ -537,6 +619,7 @@ struct rte_crypto_asym_op {
> > > >  		struct rte_crypto_mod_op_param modinv;
> > > >  		struct rte_crypto_dh_op_param dh;
> > > >  		struct rte_crypto_dsa_op_param dsa;
> > > > +		struct rte_crypto_ecdsa_op_param ecdsa;
> > > >  	};
> > > >  };
> > > >
> > > > diff --git a/lib/librte_cryptodev/rte_cryptodev.c
> > > > b/lib/librte_cryptodev/rte_cryptodev.c
> > > > index 89aa2ed..0d6babb 100644
> > > > --- a/lib/librte_cryptodev/rte_cryptodev.c
> > > > +++ b/lib/librte_cryptodev/rte_cryptodev.c
> > > > @@ -173,6 +173,7 @@ const char *rte_crypto_asym_xform_strings[] = {
> > > >  	[RTE_CRYPTO_ASYM_XFORM_MODINV]	= "modinv",
> > > >  	[RTE_CRYPTO_ASYM_XFORM_DH]	= "dh",
> > > >  	[RTE_CRYPTO_ASYM_XFORM_DSA]	= "dsa",
> > > > +	[RTE_CRYPTO_ASYM_XFORM_ECDSA]	= "ecdsa",
> > > >  };
> > > >
> > > >  /**
> > > > --
> > > > 2.7.4
> > >
> > > Regards,
> > > Arek