Re: [dpdk-dev] [EXT] [PATCH v3 8/8] examples/ipsec-secgw: add support for additional algorithms

["Nicolau, Radu" <radu.nicolau@intel.com>]

Hi Akhil, thanks for the feedback, some comments below.

On 10/8/2021 8:07 PM, Akhil Goyal wrote:
>> Add support for AES-GMAC, AES_CTR, AES_XCBC_MAC,
>> AES_CCM, CHACHA20_POLY1305
>>
>> Signed-off-by: Declan Doherty <declan.doherty@intel.com>
>> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
>> ---
>>   examples/ipsec-secgw/ipsec.h |   3 +-
>>   examples/ipsec-secgw/sa.c    | 133 ++++++++++++++++++++++++++++++++---
>>   2 files changed, 126 insertions(+), 10 deletions(-)
>>
> Documentation?
> Release notes?

I will follow up with an updated patchset.


>> -
>> +#define MAX_KEY_SIZE		96
> Max key length defined below is 64, then why 96?
I guess just to have some room, I will set it back to 64.
>
>>   /*
>>   		}
>>
>> -		if (sa->aead_algo == RTE_CRYPTO_AEAD_AES_GCM) {
>> -			iv_length = 12;
>> +
>> +		if (sa->aead_algo == RTE_CRYPTO_AEAD_AES_GCM ||
>> +			sa->aead_algo == RTE_CRYPTO_AEAD_AES_CCM ||
>> +			sa->aead_algo ==
>> RTE_CRYPTO_AEAD_CHACHA20_POLY1305) {
>> +
>> +			if (ips->type ==
>> +
>> 	RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO) {
>> +				iv_length = 8;
> How is IV length dependent on the action type?
> It should be same for all modes for a particular algorithm. Right?

The issue here with inline crypto and AEAD algorithms is that we have 
the IV and the salt used for building the nonce but only the IV is 
included in the ESP header. And technically and according to the RFCs 
the value in the inline branch is the correct one, the other is actually 
the nonce buffer length, not the IV length. I'm not sure if this is the 
proper fix, probably it's not, but it's probably a change that will not 
break it for other crypto devices.