Re: [dpdk-dev] [PATCH v4 1/4] lib/cryptodev: add asymmetric algos in cryptodev

["Doherty, Declan" <declan.doherty@intel.com>]

On 06/07/2018 3:28 PM, Verma, Shally wrote:
> Hi Declan
> 
>> -----Original Message-----
>> From: Doherty, Declan [mailto:declan.doherty@intel.com]
>> Sent: 05 July 2018 20:24
>> To: Verma, Shally <Shally.Verma@cavium.com>; pablo.de.lara.guarch@intel.com
>> Cc: 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>; Kartha,
>> Umesh <Umesh.Kartha@cavium.com>
>> Subject: Re: [dpdk-dev] [PATCH v4 1/4] lib/cryptodev: add asymmetric algos in cryptodev
>>
>> External Email
>>
>> Hey Shally,
>>
>> just a few things inline below mainly concerned with the need to be able
>> to support session-less operations in future PMDs. I think with a few
>> minor changes to the API now it should allow session-less to be
>> supported later without the need for a major rework of the APIs, I don't
>> think this should cause any major rework to your PMD just the adoption
>> of some new more explicit op types.
>>
>> Thanks
>> Declan
>>
>> On 03/07/2018 4:24 PM, Shally Verma wrote:
>>> Add rte_crypto_asym.h with supported xfrms
>>> and associated op structures and APIs
>>>
>>> API currently supports:
>>> - RSA Encrypt, Decrypt, Sign and Verify
>>> - Modular Exponentiation and Inversion
>>> - DSA Sign and Verify
>>> - Deffie-hellman private key exchange
>>> - Deffie-hellman public key exchange
>>> - Deffie-hellman shared secret compute
>>> - Deffie-hellman public/private key pair generation
>>> using xform chain
>>>
>>> Signed-off-by: Shally Verma <shally.verma@caviumnetworks.com>
>>> Signed-off-by: Sunila Sahu <sunila.sahu@caviumnetworks.com>
>>> Signed-off-by: Ashish Gupta <ashish.gupta@caviumnetworks.com>
>>> Signed-off-by: Umesh Kartha <umesh.kartha@caviumnetworks.com>
>>> ---
>>>    lib/librte_cryptodev/Makefile          |   1 +
>>>    lib/librte_cryptodev/meson.build       |   3 +-
>>>    lib/librte_cryptodev/rte_crypto_asym.h | 496 +++++++++++++++++++++++++++++++++
>>>    3 files changed, 499 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/librte_cryptodev/Makefile b/lib/librte_cr
>>
>> ...
>>
>>> +typedef struct rte_crypto_param_t {
>>> +     uint8_t *data;
>>> +     /**< pointer to buffer holding data */
>>> +     rte_iova_t iova;
>>> +     /**< IO address of data buffer */
>>> +     size_t length;
>>> +     /**< length of data in bytes */
>>> +} rte_crypto_param;
>>
>> What is the intended way for this memory to be allocated,
> 
> [Shally] It should be pointer to flat buffers and added only to input/output data to/from
> asymmetric crypto engine.
> 
>> it seems like
>> there might be a more general requirement in DPDK for IO addressable
>> memory (compression? other hardware acceleators implemented on FPGAs)
>> than just asymmetric crypto, will we end up needing to support features
>> like scatter gather lists in this structure?
> 
> [Shally] I don’t anticipate that we would need to support scatter-gather data buffers as far as it is used for asymmetric.
> And I'm not aware if we have requirement to support it for asymmetric processing since data size is usually small for
> such operations. Thus, app is expected to send linear buffers for input/output.
> 
> Does that answer your question? Or did I miss anything?
> 

Sure I understand the rationale.

> 
>> btw I think this is
>> probably fine for the moment as it will be expermential but I think it
>> will need to be addressed before the removal of the expermential tag.
>>
> 
> ...
> 
>>> +     RTE_CRYPTO_ASYM_XFORM_MODINV,
>>
>> Would prefer if this was _MOD_INV :)
>>
>>> +     /**< Modular Inverse
>>> +      * Perform Modulus inverse b^(-1) mod n
>>> +      */
>>> +     RTE_CRYPTO_ASYM_XFORM_MODEX,
>>
>> any this was _MOD_EX :)
> 
> [Shally] fine will do name change.
> 
>>
>>> +     /**< Modular Exponentiation
>>> +      * Perform Modular Exponentiation b^e mod n
>>> +      */
>>> +     RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
>>> +     /**< End of list */
>>> +};
>>> +
>>> +/**
>>> + * Asymmetric crypto operation type variants
>>> + */
>>> +enum rte_crypto_asym_op_type {
>>> +     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_PRIVATE_KEY_GENERATE,
>>> +     /**< DH Private Key generation operation */
>>> +     RTE_CRYPTO_ASYM_OP_PUBLIC_KEY_GENERATE,
>>> +     /**< DH Public Key generation operation */
>>> +     RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE,
>>> +     /**< DH Shared Secret compute operation */
>>> +     RTE_CRYPTO_ASYM_OP_LIST_END
>>> +};
>>> +
>>
>> I think that having generic operation types which may or may not apply
>> to all of the defined xforms is confusing from a user perspective and in
>> the longer term will make it impossible to support session-less
>> asymmetric operations. If we instead do something like
>>
>>         RTE_CRYPTO_ASYM_OP_RSA_ENCRYPT,
>>         RTE_CRYPTO_ASYM_OP_RSA_DECRYPT,
>>         RTE_CRYPTO_ASYM_OP_RSA_SIGN,
>>         RTE_CRYPTO_ASYM_OP_RSA_VERIFY,
>>         RTE_CRYPTO_ASYM_OP_DH_KEY_GENERATE,
>>         RTE_CRYPTO_ASYM_OP_DH_SHARED_SECRET_COMPUTE,
>>         etc...
>>
>> Then the op type becomes very explicit and will allow session-less
>> operations to be supported by PMDs. This shouldn't have any impact on
>> your current implementation other than updating the op type.
>>
> 
> [Shally] Ok, so you suggest to merge xform and op_type (including keeping op_type in one place)  for simplicity sake.
> 
> If we take this change, then I will prefer to keep it as xforms rather than op_types, such as,
> 
> enum rte_crypto_asym_xform_types {
> RTE_CRYPTO_ASYM_XFORM_RSA_ENCRYPT,
> RTE_CRYPTO_ASYM_XFORM_RSA_SIGN,
> RTE_CRYPTO_ASYM_XFORM_DSA_SIGN,
> RTE_CRYPTO_ASYM_XFORM_DSA_VERIFY,
> RTE_CRYPTO_ASYM_XFORM_DH_PRIV_KEY_GENERATE,
> RTE_CRYPTO_ASYM_XFORM_DH_PUB_KEY_GENERATE,
> RTE_CRYPTO_ASYM_XFORM_DH_SHARED_SECRET_COMPUTE, .... and so on..
> }
> 
> These, when set on a rte_crypto_asym_xform , will work seamlessly for both session and session-less.
> I also see advantage here to support xform chaining. In this case, op_type in rte_crypto_asym_xx_op_params isn't needed and will be removed.
> It will add bit of repetition though, like RSA_SIGN / DSA_SIGN, but that would occur in any case, if we are merging two types.
> Does that sound okay?

I would be a bit concerned with dropping the 
rte_crypto_asym_xx_op_params as if we are following the symmetric model, 
the xform should only specify the immutable data in relation to a 
transform and the operation should provide the mutable input data. I'm 
not sure how this would look like for the different asymmetric 
operations but if there are transforms that don't have immutable data 
then it would make more sense not to have a specific xform data 
structure for that and pass the data through that transformations 
operation structure.

> 
> We were about to submit Openssl PMD with asym support today. But I would hold back till we align on this.
> ...
> 

Ok, I think I had missed part of the complexity of the chained used 
case. I think if we are to follow the symmetric crypto model we end up 
with something along the lines of a fundamental op types - dh, dsa, mod, 
rsa etc.

enum rte_crypto_asym_op_types {
	RTE_CRYPTO_ASYM_OP_DH,
	RTE_CRYPTO_ASYM_OP_DSA,
	RTE_CRYPTO_ASYM_OP_MOD,
	RTE_CRYPTO_ASYM_OP_RSA,
};

with a corresponding asym_op as follows.

struct rte_crypto_asym_op {
	union {
		struct rte_cryptodev_asym_session *session;
		/**< Handle for the initialized session context */
		struct rte_crypto_asym_xform *xform;
	};

	union {
		struct rte_crypto_asym_dh_op_data dh;
		struct rte_crypto_asym_dsa_op_data dsa;
		struct rte_crypto_asyn_mod_op_data mod;
		struct rte_crypto_asym_rsa_op_data rsa;
	} op_data;
};


In terms of the xform definitions I'm not sure that we should have all 
the different transforms defined in the same enum. If we take the below 
approach, all the specifics of each transform could be split out into 
separate headers, giving a cleaner API. This would see a xform types 
enum which would mirror the operation types:

enum rte_crypto_asym_xform_types {
	RTE_CRYPTO_ASYM_XFORM_DH,
	RTE_CRYPTO_ASYM_XFORM_DSA,
	RTE_CRYPTO_ASYM_XFORM_MOD,
	RTE_CRYPTO_ASYM_XFORM_RSA,
}

with corresponding xforms structures

struct rte_crypto_asym_xform dh;
struct rte_crypto_asym_xform dsa;
struct rte_crypto_asym_xform modlus;
struct rte_crypto_asym_xform rsa;

then within the xforms would define the sub-type of that xform and have 
definitions of  the relevant session data which is required, with the 
rsa it might look like something like this:

enum rte_crypto_asym_xform_rsa_types {
	RTE_CRYPTO_ASYM_XFORM_RSA_ENCRYPT,
	RTE_CRYPTO_ASYM_XFORM_RSA_DECRYPT,
	RTE_CRYPTO_ASYM_XFORM_RSA_SIGN,
	RTE_CRYPTO_ASYM_XFORM_RSA_VERIFY,
}

struct rte_crypto_asym_xform rsa {
	enum rte_crypto_asym_op_rsa_types type;
	union {
		struct rte_crypto_asym_xform_rsa_encrypt_data encrypt;
		struct rte_crypto_asym_xform_rsa_decrypt_data decrypt;
		etc...
	} data;
};

The need for xform sub-type data structure would be dependent on whether 
there was immutable data associated with a particular transform. Also if 
we take this approach it would allow each operation type to be separated 
out into there own headers.

>> ....
>>
>>
>>> + */
>>> +struct rte_crypto_dh_xform {
>>> +     enum rte_crypto_asym_op_type type;
>>> +     /**< Setup xform for key generate or shared secret compute */
>>> +
>>
>> there is an inconsistency here in terms of were the op_type is defined,
>> in this case it is in the xform but it other cases RSA, DSA it is
>> defined in the operation information itself. I don't know of any reason
>> why it is needed in the xform but I think it must be consistent across
>> all operations/xforms. Ideally from my perspective it would be in the
>> rte_crypto_asym_op structure, see below, as this would allow
>> session/session-less operations to be supported seamlessly.
>>
> 
> [Shally] Reason was xform chaining. As per Fiona feedback, we identified requirement to break DH Key pair
> generation in to two  : PRIV_KEY_GENERATION and PUB_KEY_GENERATION, so I had to move this param around.
> But this will be addressed once we change xform_types as per suggestion above.
> 
> ...
> 
>>
>> ...
>>
>>> +/**
>>> + * Asymmetric Cryptographic Operation.
>>> + *
>>> + * Structure describing asymmetric crypto operation params.
>>> + *
>>> + */
>>> +struct rte_crypto_asym_op {
>>> +     struct rte_cryptodev_asym_session *session;
>>> +     /**< Handle for the initialised session context */
>>> +
>>> +     __extension__
>>> +     union {
>>> +             struct rte_crypto_rsa_op_param rsa;
>>> +             struct rte_crypto_mod_op_param modex;
>>> +             struct rte_crypto_mod_op_param modinv;
>>> +             struct rte_crypto_dh_op_param dh;
>>> +             struct rte_crypto_dsa_op_param dsa;
>>> +     };
>>> +} __rte_cache_aligned;
>>> +
>> Relating to my comment on position of the op_type and the minor change
>> of having an union of session/xform in the rte_crypto_asym_op structure
>> would then enable sessionless support to be added seamless in the future
>> with minimal effect to the current proposal.
> 
> [Shally] Again, this will also be resolved with change to xform_types
> 
>>
>> struct rte_crypto_asym_op {
>> -       struct rte_cryptodev_asym_session *session;
>> -       /**< Handle for the initialised session context */
>> +       enum rte_crypto_asym_op_type op_type;
>> +
>> +       union {
>> +               struct rte_cryptodev_asym_session *session;
>> +               /**< Handle for the initialised session context */
>> +               struct rte_crypto_asym_xform *xform;
>> +       };
>>
> [Shally] Ok. Will add this change. But then I'll have to check if crypto PMD has support to reflect which type of mode it supports?
> 
The sess_type=RTE_CRYPTO_OP_WITH_SESSION/RTE_CRYPTO_OP_SESSIONLESS 
should be set in the outer crypto_op struct, so if the PMD doesn't 
support the selected sess_type, it should just fail the enqueue and 
possibly log an error.

> Thanks for review.
> Shally
> 
>>          __extension__
>>
>>
>>> +#ifdef __cplusplus
>>> +}
>>> +#endif
>>> +
>>> +#endif /* _RTE_CRYPTO_ASYM_H_ */
>>>
>