GBalakrishna replied on github web page: platform/linux-dpdk/odp_crypto.c line 26 @@ -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; break; case ODP_AUTH_ALG_SHA256_HMAC: #if ODP_DEPRECATED_API case ODP_AUTH_ALG_SHA256_128: #endif auth_xform->auth.algo = RTE_CRYPTO_AUTH_SHA256_HMAC; - auth_xform->auth.digest_length = 16;
Comment: 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_r161782774 updated_at 2018-01-16 15:03:20