Dmitry Eremin-Solenikov(lumag) replied on github web page:
platform/linux-dpdk/odp_crypto.c
line 333
@@ -855,48 +924,51 @@ int odp_crypto_session_create(odp_crypto_session_param_t
*param,
auth_xform.auth.key.length = 0;
}
+ auth_xform.auth.digest_length = 0;
+ if (param->auth_digest_len)
+ auth_xform.auth.digest_length = param->auth_digest_len;
/* Derive order */
- if (ODP_CRYPTO_OP_ENCODE == param->op)
- entry->do_cipher_first = param->auth_cipher_text;
- else
- entry->do_cipher_first = !param->auth_cipher_text;
-
- /* Process based on cipher */
- /* Derive order */
- if (entry->do_cipher_first) {
+ if (ODP_CRYPTO_OP_ENCODE == param->op) {
cipher_xform.cipher.op = RTE_CRYPTO_CIPHER_OP_ENCRYPT;
auth_xform.auth.op = RTE_CRYPTO_AUTH_OP_GENERATE;
- first_xform = &cipher_xform;
- first_xform->next = &auth_xform;
+ entry->do_cipher_first = param->auth_cipher_text;
} else {
cipher_xform.cipher.op = RTE_CRYPTO_CIPHER_OP_DECRYPT;
auth_xform.auth.op = RTE_CRYPTO_AUTH_OP_VERIFY;
- first_xform = &auth_xform;
- first_xform->next = &cipher_xform;
- }
-
- rc = cipher_alg_odp_to_rte(param->cipher_alg, &cipher_xform);
-
- /* Check result */
- if (rc) {
- *status = ODP_CRYPTO_SES_CREATE_ERR_INV_CIPHER;
- return -1;
+ entry->do_cipher_first = !param->auth_cipher_text;
}
- rc = auth_alg_odp_to_rte(param->auth_alg, &auth_xform);
-
- /* Check result */
- if (rc) {
- *status = ODP_CRYPTO_SES_CREATE_ERR_INV_AUTH;
- /* remove the crypto_session_entry_t */
- memset(entry, 0, sizeof(*entry));
- free_session(entry);
- return -1;
+ /* Process based on cipher */
+ /* Derive order */
+ if (entry->do_cipher_first) {
Comment:
Push this logic to `set_chain_order`
> Dmitry Eremin-Solenikov(lumag) wrote:
> Make it return `crypto_chain_order`.
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> No need to keep this under `if (iv_length)`
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> Liked this typo. It shows that the code never worked as expected.
>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>> same issue here.
>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>> Same issue here.
>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>> `auth.digest_length` should be set for MD5_96 here, but not for MD5_HMAC.
>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>> It's really not a problem at this moment, this can be fixed later.
>>>>>>>> GBalakrishna wrote
>>>>>>>> oke I see. I will make the update in the next version.
>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>> both = Encrypt-then-MAC and MAC-cleartext
>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>> Could you please add tests cases rather than modify existing ones?
>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>> Implementation should not set digest len for cases other than
>>>>>>>>>>> several deprecated auth algos.
>>>>>>>>>>>> GBalakrishna wrote
>>>>>>>>>>>> setting digest_length is moved to get_crypto_dev(), where it
>>>>>>>>>>>> reads from PMD and set the min digest length if application
>>>>>>>>>>>> doesn't set it. Reagrding the QAT, I need to look at it and
>>>>>>>>>>>> understand. may be as a separate PR.
>>>>>>>>>>>>> GBalakrishna wrote
>>>>>>>>>>>>> I have updated the tests so that it works with both the platforms
>>>>>>>>>>>>> linux-generci & linux-dpdk.
>>>>>>>>>>>>>> GBalakrishna wrote
>>>>>>>>>>>>>> what do you mean by testing both combinations ?. We are testing
>>>>>>>>>>>>>> both the combinations ENCODE & DECODE. It's the default value we
>>>>>>>>>>>>>> are changing it.
>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>> Checkpatch flags this line:
>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>> WARNING: line over 80 characters
>>>>>>>>>>>>>>> #363: FILE: platform/linux-dpdk/test/wrapper-script.sh:3:
>>>>>>>>>>>>>>> +export ODP_PLATFORM_PARAMS=${ODP_PLATFORM_PARAMS:--n 4 --vdev
>>>>>>>>>>>>>>> "crypto_openssl" --vdev "crypto_null"}
>>>>>>>>>>>>>>> total: 0 errors, 1 warnings, 0 checks, 530 lines checked
>>>>>>>>>>>>>>> NOTE: Ignored message types: AVOID_EXTERNS BIT_MACRO
>>>>>>>>>>>>>>> COMPARISON_TO_NULL DEPRECATED_VARIABLE NEW_TYPEDEFS
>>>>>>>>>>>>>>> PREFER_PRINTF PREFER_SCANF SPLIT_STRING SSCANF_TO_KSTRTO
>>>>>>>>>>>>>>> VOLATILE
>>>>>>>>>>>>>>> 0001-linux-dpdk-crypto-support-for-cipher-auth-only-featu.patch
>>>>>>>>>>>>>>> has style problems, please review.
>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>> I suggest to factor out cipher and auth checks as separate
>>>>>>>>>>>>>>>> functions and apply them only if corresponding algorithm is
>>>>>>>>>>>>>>>> not NULL. Code would be simpler.
>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>> For deprecated MD5_96 (and SHA256_128/AES128_GCM) application
>>>>>>>>>>>>>>>>> won't set `auth.digest_len`, so you have to enforce 12 and 16
>>>>>>>>>>>>>>>>> bytes digest len. For MD5_HMAC/SHA256_HMAC (and others)
>>>>>>>>>>>>>>>>> application will set `auth.digest_len`.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> IIRC QAT driver should support truncated digests out-of-box.
>>>>>>>>>>>>>>>>> Also you can try expanding pmd_openssl to support truncated
>>>>>>>>>>>>>>>>> lengths, it should not be very complicated. Also note, that
>>>>>>>>>>>>>>>>> full-length HMAC won't be useful for IPsec.
>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>> "Do not change tests". See my previous comment.
>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>> Theoretically we'd better be testing both combinations. The
>>>>>>>>>>>>>>>>>>> problem with test changes in your commit is that you change
>>>>>>>>>>>>>>>>>>> tests rather than expand them. Can you import tests from
>>>>>>>>>>>>>>>>>>> #379?
>>>>>>>>>>>>>>>>>>>> GBalakrishna wrote
>>>>>>>>>>>>>>>>>>>> not sure what do u mean here.
>>>>>>>>>>>>>>>>>>>>> GBalakrishna wrote
>>>>>>>>>>>>>>>>>>>>> Not sure if I have fallowed. I introduced Chain order
>>>>>>>>>>>>>>>>>>>>> because dpdk PMD's doesn't have support for combination
>>>>>>>>>>>>>>>>>>>>> of NULL + Valid algo. So linux-dpdk implementation
>>>>>>>>>>>>>>>>>>>>> converts the NULL also to chain order as if it cipher
>>>>>>>>>>>>>>>>>>>>> only & auth only.
>>>>>>>>>>>>>>>>>>>>>> GBalakrishna wrote
>>>>>>>>>>>>>>>>>>>>>> my understanding of auth_cipher_text = true means do
>>>>>>>>>>>>>>>>>>>>>> encode then authenticate. and when it is applied
>>>>>>>>>>>>>>>>>>>>>> together with if (ODP_CRYPTO_OP_ENCODE == param->op)
>>>>>>>>>>>>>>>>>>>>>> sets the "entry->do_cipher_first" to true by default
>>>>>>>>>>>>>>>>>>>>>> and if the param->op == DECODE it sets the
>>>>>>>>>>>>>>>>>>>>>> !auth_cipher_text. I felt this is more easier for
>>>>>>>>>>>>>>>>>>>>>> understanding.
>>>>>>>>>>>>>>>>>>>>>>> GBalakrishna wrote
>>>>>>>>>>>>>>>>>>>>>>> openssl PMD in 17.02 supports the digest length to be
>>>>>>>>>>>>>>>>>>>>>>> min 16 bytes. But I have now removed the setting
>>>>>>>>>>>>>>>>>>>>>>> digest_lengths from here as it sets it from the
>>>>>>>>>>>>>>>>>>>>>>> application.
>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>> why?
>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>> `non tangere testos meos`
>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>> I'd suggest to use ALG_NULL checks directly, rather
>>>>>>>>>>>>>>>>>>>>>>>>>> than introduce chain order.
>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>> This is wrong. MD5_96 should use 12-byte digest
>>>>>>>>>>>>>>>>>>>>>>>>>>> length. MD5_HMAC should use digest length specified
>>>>>>>>>>>>>>>>>>>>>>>>>>> by application.
https://github.com/Linaro/odp/pull/385#discussion_r161767021
updated_at 2018-01-16 14:21:10
