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:
you mean to say for MD5_96, we should ignore the digest_length from application
even if it is set ?
> GBalakrishna wrote
> you mean to make this in separate commit ?
>> GBalakrishna wrote
>> you mean is_valid_size() ?
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> 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_r161802989
updated_at 2018-01-16 16:02:44
