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.kusz...@intel.com>
> Sent: Thursday, January 9, 2020 6:34 PM
> To: Anoob Joseph <ano...@marvell.com>; Akhil Goyal <akhil.go...@nxp.com>;
> Doherty, Declan <declan.dohe...@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.gua...@intel.com>
> Cc: Ayuj Verma <ayve...@marvell.com>; Trahe, Fiona
> <fiona.tr...@intel.com>; Jerin Jacob Kollanukkaran <jer...@marvell.com>;
> Narayana Prasad Raju Athreya <pathr...@marvell.com>; Shally Verma
> <shal...@marvell.com>; Ankur Dwivedi <adwiv...@marvell.com>; Sunila Sahu
> <ss...@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 <ano...@marvell.com>
> > Sent: Thursday, January 2, 2020 8:55 AM
> > To: Kusztal, ArkadiuszX <arkadiuszx.kusz...@intel.com>; Akhil Goyal
> > <akhil.go...@nxp.com>; Doherty, Declan <declan.dohe...@intel.com>; De
> > Lara Guarch, Pablo <pablo.de.lara.gua...@intel.com>
> > Cc: Ayuj Verma <ayve...@marvell.com>; Trahe, Fiona
> > <fiona.tr...@intel.com>; Jerin Jacob Kollanukkaran
> > <jer...@marvell.com>; Narayana Prasad Raju Athreya
> > <pathr...@marvell.com>; Shally Verma <shal...@marvell.com>; Ankur
> > Dwivedi <adwiv...@marvell.com>; Sunila Sahu <ss...@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.kusz...@intel.com>
> > > Sent: Friday, December 20, 2019 9:36 PM
> > > To: Anoob Joseph <ano...@marvell.com>; Akhil Goyal
> > > <akhil.go...@nxp.com>; Doherty, Declan <declan.dohe...@intel.com>;
> > > De Lara Guarch, Pablo <pablo.de.lara.gua...@intel.com>
> > > Cc: Ayuj Verma <ayve...@marvell.com>; Trahe, Fiona
> > > <fiona.tr...@intel.com>; Jerin Jacob Kollanukkaran
> > > <jer...@marvell.com>; Narayana Prasad Raju Athreya
> > > <pathr...@marvell.com>; Shally Verma <shal...@marvell.com>; Ankur
> > > Dwivedi <adwiv...@marvell.com>; Sunila Sahu <ss...@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
