Dmitry Eremin-Solenikov(lumag) 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:
OK. You always use `param->auth_digest_len` if it is set. This logic is
incorrect. It should be used for all cases except MD5_96, SHA256_128,
AES128_GCM (where an override should be used).
> GBalakrishna wrote
> yes it works.
>> GBalakrishna wrote
>> it is set in get_crypto_dev()
>>> GBalakrishna wrote
>>> it is set in get_crypto_dev()
>>>> GBalakrishna wrote
>>>> 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_r161795465
updated_at 2018-01-16 15:40:24
