Re: [dpdk-dev] [RFC] cryptodev/asymm: propose changes to modexp and modinv API

["Verma, Shally" <Shally.Verma@cavium.com>]

HI Arek, Fiona

>-----Original Message-----
>From: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>
>Sent: 17 December 2018 19:55
>To: Verma, Shally <Shally.Verma@cavium.com>
>Cc: dev@dpdk.org; Trahe, Fiona <fiona.trahe@intel.com>; Doherty, Declan <declan.doherty@intel.com>; Kanaka Durga Kotamarthy
><kkotamarthy@marvell.com>; Sunila Sahu <ssahu@marvell.com>; Kotamarthy, Kanaka <Kanaka.Kotamarthy@cavium.com>; Sahu,
>Sunila <Sunila.Sahu@cavium.com>; Cel, TomaszX <tomaszx.cel@intel.com>; Jozwiak, TomaszX <tomaszx.jozwiak@intel.com>
>Subject: RE: [RFC] cryptodev/asymm: propose changes to modexp and modinv API
>
>External Email
>
>Hi Shally,
>
>Thanks for your answers :).
>
>My answers in [AK-v2]
>
>> -----Original Message-----
>> From: Verma, Shally [mailto:Shally.Verma@cavium.com]
>> Sent: Monday, December 17, 2018 6:45 AM
>> To: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>
>> Cc: dev@dpdk.org; Trahe, Fiona <fiona.trahe@intel.com>; Doherty, Declan
>> <declan.doherty@intel.com>; Kanaka Durga Kotamarthy
>> <kkotamarthy@marvell.com>; Sunila Sahu <ssahu@marvell.com>;
>> Kotamarthy, Kanaka <Kanaka.Kotamarthy@cavium.com>; Sahu, Sunila
>> <Sunila.Sahu@cavium.com>
>> Subject: RE: [RFC] cryptodev/asymm: propose changes to modexp and
>> modinv API
>>
>> HI Arek
>>
>> Sorry for late response. Please see response inline
>>
>> From: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>
>> Sent: 13 December 2018 01:56
>> To: Verma, Shally <Shally.Verma@cavium.com>
>> Cc: dev@dpdk.org; Trahe, Fiona <fiona.trahe@intel.com>; Doherty, Declan
>> <declan.doherty@intel.com>
>> Subject: [RFC] cryptodev/asymm: propose changes to modexp and modinv
>> API
>>
>> External Email
>> Hi Shally,
>>
>> I'm implementing a crypto asymmetric PMD and have some concerns about
>> the API which I will work through over the next few months. Starting with
>> modexp and modinv I have the following questions / suggestions:
>>
>>               rte_crypto_asym.h:233
>>                              rte_crypto_param modulus;
>>                              /**< modulus
>>                              * Prime modulus of the modexp transform operation in
>> octet-string
>>                              * network byte order format.
>>                              */
>>                              [AK] - Why prime? RSA for example use semi-prime or "RSA
>> multi-prime".
>>                              It should be just any positive integer.
>> [Shally] Hmm.. yes you're right . by the purpose of it , it is a semi-prime
>> input.. so prime shouldn't be mentioned here.
>[AK-v2] I think it could be any nonzero number even composite, for DH, DSA it would be prime etc.
>>                              [AK] - If session API layer should check if it is non-zero and
>> set flag accordingly.
>> [Shally] Sorry I didn't get this.. which flag you mean here? if modulus value 0
>> is passed, it should be considered as INVALID_PARAM.
>[AK-v2] - INVALID_PARAM is perfectly fine for me :).

[Shally] Just to club Fiona response here to which I agree. If you intend to check modulus value to 0 in session_init, then cryptodev lib asym_session_configure API  can check for Invalid Param.
For any invalid op during enqueue/dequeue it should be in PMD. 

>>
>>               rte_crypto_asym.h:253
>>                              rte_crypto_param modulus;
>>                              /**<
>>                              * Pointer to the prime modulus data for modular
>>                              * inverse operation in octet-string network byte
>>                              * order format.
>>                              */
>>                              [AK] - Same situation as for mod exp. Just any number.
>> [Shally] Yea. It should be reworded as modulus data instead of *prime*
>> modulus data
>>
>>                              For example when using with RSA Carmichael and Euler
>> Totient function will even
>>                              have composite factors.
>>
>>               rte_crypto_asym.h:323
>>                              struct rte_crypto_mod_op_param {
>>                              [AK] - There should be a result field. It size should be equal to
>> the size
>>                              of modulus. Same apply to mod mult inverse. It should be
>> driver responsability to check if result
>>                              will not overflow [Shally] so these are in-place operation.
>> Output will be written back to base param. It also imply length of allocated
>> array should be >= modulus length which is passed in session param.
>[AK-v2] I would abandon in-place/oop approach at all in asymmetric. For symmetric reason for in-place is that very often structure of
>packet is almost intact (macs, ip addresses, ttl etc are changed but structure remains the same, it may differ for IPSec ESP mode etc).
>For asymmetric it is mainly used for handshakes (for example in TLS RSA use case client will send 48byte of pre master secret which
>server will use to generate master secret after decryption). I generally don't think asymmetric crypto can be used as symmetric
>especially that only RSA would be (to some extent) capable of it.

[Shally] So you suggest all asym ops should be out of place? Am okay with add that. However would like to ask if anyone has preference to keep in-place option in Asym.
If so, then we would need to add Feature flag indicating in-place processing capability.

>>
>>                              [AK] - Any particular reason modulus and exponent is in
>> session? Not saying
>>                              it is wrong but is it for DH, RSA use cases only?
>> [Shally] no that's not the intent. For RSA and DH respective xforms have been
>> defined. It is kept in session envisioning modulus and exponent wont change
>> frequently across operation but only base value.
>> So once context is loaded with modulus and exponent , app can call modexp
>> on different base values.
>>
>>                              rte_crypto.h:39
>>                              enum rte_crypto_op_status {
>>                              [AK] - There will be many more status options in asymmetric,
>>                              could we probably create new one for asymmetric crypto?
>> Even if asymmetric and symmetric
>>                              overlap?
>>                              For mod exp, mod inv potentially it will be:
>>                             DIVIDING_BY_ZERO_ERROR, INVERSE_NOT_EXISTS_ERROR...
>> [Shally] So far RTE_CRYPTO_OP_STATUS_INVALID_PARAM has been
>> sufficient for such cases. Do you have any use-cases where you need specific
>> error code to indicate asym specific error codes?
>There would be many examples, one of which:
>[AK-v2] Ok, still to discussion i think though.
>>
>>               rte_crypto_asym.h:33
>>                              size_t length;
>>                              /**< length of data in bytes */
>>                              [AK] - Is it guaranteed to be length of actual data, not
>> allocated memory (i mean no leading 0ed bytes), so the most significant bit
>> will be in data[0]?
>> [Shally] it should be length of actual data not length of allocated memory to
>> an array.
>> However it might create bit confusion on modular exponentiation op param
>> as that expect length passed should tell actual data length in base array but
>> array itself should be allocated upto modulus length.
>[AK-v2] - so it is maybe good idea to have allocated data in bytes and actual len in bits here.

[Shally] No that will make it complex and breaks compatibility too. I would propose to keep it in bytes which states length of actual data present in array.
Any confusion around it will be resolved if we add out of place or proper documentation if in-place is retained.

I would suggest you to send a patch with things that we agree or you propose. We can discuss on that further.

Thanks
Shally
>>
>>                              [AK] - could it be uint16/32_t instead as size_t can have
>> different sizes in different implementations, uint16_t should be enough
>>                              for all algorithms big integer sizes [Shally] no hard choices
>> here though. But size_t would never be less than uint16_t so it guarantee to
>> be large enough for any machines
>>
>>               rte_crypto_asym.h:74, 250, 257, 351
>>                              /**< Modular Inverse
>>                              [AK] - Modular Multiplicative Inverse
>>     [Shally] Ack.
>>
>> Thanks,
>> Arek
>>
>>