GBalakrishna replied on github web page:
platform/linux-dpdk/odp_crypto.c
line 19
@@ -122,29 +130,24 @@ static int auth_alg_odp_to_rte(odp_auth_alg_t auth_alg,
case ODP_AUTH_ALG_MD5_96:
#endif
auth_xform->auth.algo = RTE_CRYPTO_AUTH_MD5_HMAC;
- auth_xform->auth.digest_length = 12;
Comment:
it is set in get_crypto_dev(): auth_xform->auth.digest_length =
cap->sym.auth.digest_size.min
> Dmitry Eremin-Solenikov(lumag) wrote:
> All test updates should go in separate commit.
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> Name of the test suggests, that test is using truncated hmac, but then you
>> specify full auth_digest_len.
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> and this
>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>> Will it work w/o crypto_null driver?
>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>> `res` does not quite follow `crypto_chain_order`, does it? Please rename
>>>>> to 'order' or smth. like that.
>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>> 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_r161782750
updated_at 2018-01-16 15:03:14
