[PATCH 22.11] crypto/ipsec_mb: fix incorrectly setting cipher keys

[PATCH 22.11] crypto/ipsec_mb: fix incorrectly setting cipher keys

[Ciara Power]

The encryption and decryption keys were incorrectly being reset based on
authentication algorithm after already being set earlier in the code
based on cipher algorithm.
In cases when 3DES was used, the keys were being
incorrectly overwritten.

For CPU path, there is no need to have the keys set for XCBC and CMAC
cases.

Fixes: 010230a1543b ("crypto/aesni_mb: support Chacha20-Poly1305")
Fixes: b0a37e8cd2ac ("crypto/ipsec_mb: fix cipher key setting")
Fixes: a2c6d3f34f90 ("crypto/aesni_mb: support CPU crypto")

Signed-off-by: Ciara Power <ciara.power@intel.com>
---
Cc: pablo.de.lara.guarch@intel.com
---
 drivers/crypto/ipsec_mb/pmd_aesni_mb.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/crypto/ipsec_mb/pmd_aesni_mb.c b/drivers/crypto/ipsec_mb/pmd_aesni_mb.c
index ac20d01937..02d419b587 100644
--- a/drivers/crypto/ipsec_mb/pmd_aesni_mb.c
+++ b/drivers/crypto/ipsec_mb/pmd_aesni_mb.c
@@ -1017,9 +1017,6 @@ set_cpu_mb_job_params(IMB_JOB *job, struct aesni_mb_session *session,
 		job->u.XCBC._k1_expanded = session->auth.xcbc.k1_expanded;
 		job->u.XCBC._k2 = session->auth.xcbc.k2;
 		job->u.XCBC._k3 = session->auth.xcbc.k3;
-
-		job->enc_keys = session->cipher.expanded_aes_keys.encode;
-		job->dec_keys = session->cipher.expanded_aes_keys.decode;
 		break;
 
 	case IMB_AUTH_AES_CCM:
@@ -1034,8 +1031,6 @@ set_cpu_mb_job_params(IMB_JOB *job, struct aesni_mb_session *session,
 		job->u.CMAC._key_expanded = session->auth.cmac.expkey;
 		job->u.CMAC._skey1 = session->auth.cmac.skey1;
 		job->u.CMAC._skey2 = session->auth.cmac.skey2;
-		job->enc_keys = session->cipher.expanded_aes_keys.encode;
-		job->dec_keys = session->cipher.expanded_aes_keys.decode;
 		break;
 
 	case IMB_AUTH_AES_GMAC:
@@ -1331,24 +1326,17 @@ set_mb_job_params(IMB_JOB *job, struct ipsec_mb_qp *qp,
 		job->u.XCBC._k1_expanded = session->auth.xcbc.k1_expanded;
 		job->u.XCBC._k2 = session->auth.xcbc.k2;
 		job->u.XCBC._k3 = session->auth.xcbc.k3;
-
-		job->enc_keys = session->cipher.expanded_aes_keys.encode;
-		job->dec_keys = session->cipher.expanded_aes_keys.decode;
 		break;
 
 	case IMB_AUTH_AES_CCM:
 		job->u.CCM.aad = op->sym->aead.aad.data + 18;
 		job->u.CCM.aad_len_in_bytes = session->aead.aad_len;
-		job->enc_keys = session->cipher.expanded_aes_keys.encode;
-		job->dec_keys = session->cipher.expanded_aes_keys.decode;
 		break;
 
 	case IMB_AUTH_AES_CMAC:
 		job->u.CMAC._key_expanded = session->auth.cmac.expkey;
 		job->u.CMAC._skey1 = session->auth.cmac.skey1;
 		job->u.CMAC._skey2 = session->auth.cmac.skey2;
-		job->enc_keys = session->cipher.expanded_aes_keys.encode;
-		job->dec_keys = session->cipher.expanded_aes_keys.decode;
 		break;
 
 	case IMB_AUTH_AES_GMAC:
@@ -1396,8 +1384,6 @@ set_mb_job_params(IMB_JOB *job, struct ipsec_mb_qp *qp,
 			job->cipher_mode = IMB_CIPHER_CHACHA20_POLY1305_SGL;
 			job->hash_alg = IMB_AUTH_CHACHA20_POLY1305_SGL;
 		}
-		job->enc_keys = session->cipher.expanded_aes_keys.encode;
-		job->dec_keys = session->cipher.expanded_aes_keys.encode;
 		break;
 	default:
 		job->u.HMAC._hashed_auth_key_xor_ipad =
-- 
2.25.1

Re: [PATCH 22.11] crypto/ipsec_mb: fix incorrectly setting cipher keys

[Luca Boccassi]

On Fri, 5 Apr 2024 at 11:46, Ciara Power <ciara.power@intel.com> wrote:
>
> The encryption and decryption keys were incorrectly being reset based on
> authentication algorithm after already being set earlier in the code
> based on cipher algorithm.
> In cases when 3DES was used, the keys were being
> incorrectly overwritten.
>
> For CPU path, there is no need to have the keys set for XCBC and CMAC
> cases.
>
> Fixes: 010230a1543b ("crypto/aesni_mb: support Chacha20-Poly1305")
> Fixes: b0a37e8cd2ac ("crypto/ipsec_mb: fix cipher key setting")
> Fixes: a2c6d3f34f90 ("crypto/aesni_mb: support CPU crypto")
>
> Signed-off-by: Ciara Power <ciara.power@intel.com>
> ---
> Cc: pablo.de.lara.guarch@intel.com
> ---
>  drivers/crypto/ipsec_mb/pmd_aesni_mb.c | 14 --------------
>  1 file changed, 14 deletions(-)

I have already tagged rc1 - is this fixing a regression introduced in
rc1 itself? If not, how important is it, could it wait for the next
release?

RE: [PATCH 22.11] crypto/ipsec_mb: fix incorrectly setting cipher keys

[Power, Ciara]

Hi Luca,

> -----Original Message-----
> From: Luca Boccassi <luca.boccassi@gmail.com>
> Sent: Friday, April 5, 2024 3:44 PM
> To: Power, Ciara <ciara.power@intel.com>
> Cc: stable@dpdk.org; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> Ji, Kai <kai.ji@intel.com>
> Subject: Re: [PATCH 22.11] crypto/ipsec_mb: fix incorrectly setting cipher keys
> 
> On Fri, 5 Apr 2024 at 11:46, Ciara Power <ciara.power@intel.com> wrote:
> >
> > The encryption and decryption keys were incorrectly being reset based
> > on authentication algorithm after already being set earlier in the
> > code based on cipher algorithm.
> > In cases when 3DES was used, the keys were being incorrectly
> > overwritten.
> >
> > For CPU path, there is no need to have the keys set for XCBC and CMAC
> > cases.
> >
> > Fixes: 010230a1543b ("crypto/aesni_mb: support Chacha20-Poly1305")
> > Fixes: b0a37e8cd2ac ("crypto/ipsec_mb: fix cipher key setting")
> > Fixes: a2c6d3f34f90 ("crypto/aesni_mb: support CPU crypto")
> >
> > Signed-off-by: Ciara Power <ciara.power@intel.com>
> > ---
> > Cc: pablo.de.lara.guarch@intel.com
> > ---
> >  drivers/crypto/ipsec_mb/pmd_aesni_mb.c | 14 --------------
> >  1 file changed, 14 deletions(-)
> 
> I have already tagged rc1 - is this fixing a regression introduced in
> rc1 itself? If not, how important is it, could it wait for the next release?

No, it is fixing an issue that existed before 22.11 itself. I have also sent the fix for 21.11 LTS.
The bug was reported by an external user as it caused seg faults for their algorithm use case, so the sooner the better for fix to be merged.
Would it be suitable for merge in rc2?

Thanks,
Ciara

Re: [PATCH 22.11] crypto/ipsec_mb: fix incorrectly setting cipher keys

[Luca Boccassi]

On Mon, 8 Apr 2024 at 08:17, Power, Ciara <ciara.power@intel.com> wrote:
>
> Hi Luca,
>
> > -----Original Message-----
> > From: Luca Boccassi <luca.boccassi@gmail.com>
> > Sent: Friday, April 5, 2024 3:44 PM
> > To: Power, Ciara <ciara.power@intel.com>
> > Cc: stable@dpdk.org; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> > Ji, Kai <kai.ji@intel.com>
> > Subject: Re: [PATCH 22.11] crypto/ipsec_mb: fix incorrectly setting cipher keys
> >
> > On Fri, 5 Apr 2024 at 11:46, Ciara Power <ciara.power@intel.com> wrote:
> > >
> > > The encryption and decryption keys were incorrectly being reset based
> > > on authentication algorithm after already being set earlier in the
> > > code based on cipher algorithm.
> > > In cases when 3DES was used, the keys were being incorrectly
> > > overwritten.
> > >
> > > For CPU path, there is no need to have the keys set for XCBC and CMAC
> > > cases.
> > >
> > > Fixes: 010230a1543b ("crypto/aesni_mb: support Chacha20-Poly1305")
> > > Fixes: b0a37e8cd2ac ("crypto/ipsec_mb: fix cipher key setting")
> > > Fixes: a2c6d3f34f90 ("crypto/aesni_mb: support CPU crypto")
> > >
> > > Signed-off-by: Ciara Power <ciara.power@intel.com>
> > > ---
> > > Cc: pablo.de.lara.guarch@intel.com
> > > ---
> > >  drivers/crypto/ipsec_mb/pmd_aesni_mb.c | 14 --------------
> > >  1 file changed, 14 deletions(-)
> >
> > I have already tagged rc1 - is this fixing a regression introduced in
> > rc1 itself? If not, how important is it, could it wait for the next release?
>
> No, it is fixing an issue that existed before 22.11 itself. I have also sent the fix for 21.11 LTS.
> The bug was reported by an external user as it caused seg faults for their algorithm use case, so the sooner the better for fix to be merged.
> Would it be suitable for merge in rc2?

I am not planning an rc2, but if you can confirm you have tested this
patch on top of rc1 on the affected platform then I can merge it for
the final release.

RE: [PATCH 22.11] crypto/ipsec_mb: fix incorrectly setting cipher keys

[Power, Ciara]

> -----Original Message-----
> From: Luca Boccassi <bluca@debian.org>
> Sent: Monday, April 8, 2024 10:23 AM
> To: Power, Ciara <ciara.power@intel.com>
> Cc: stable@dpdk.org; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> Ji, Kai <kai.ji@intel.com>
> Subject: Re: [PATCH 22.11] crypto/ipsec_mb: fix incorrectly setting cipher keys
> 
> On Mon, 8 Apr 2024 at 08:17, Power, Ciara <ciara.power@intel.com> wrote:
> >
> > Hi Luca,
> >
> > > -----Original Message-----
> > > From: Luca Boccassi <luca.boccassi@gmail.com>
> > > Sent: Friday, April 5, 2024 3:44 PM
> > > To: Power, Ciara <ciara.power@intel.com>
> > > Cc: stable@dpdk.org; De Lara Guarch, Pablo
> > > <pablo.de.lara.guarch@intel.com>; Ji, Kai <kai.ji@intel.com>
> > > Subject: Re: [PATCH 22.11] crypto/ipsec_mb: fix incorrectly setting
> > > cipher keys
> > >
> > > On Fri, 5 Apr 2024 at 11:46, Ciara Power <ciara.power@intel.com> wrote:
> > > >
> > > > The encryption and decryption keys were incorrectly being reset
> > > > based on authentication algorithm after already being set earlier
> > > > in the code based on cipher algorithm.
> > > > In cases when 3DES was used, the keys were being incorrectly
> > > > overwritten.
> > > >
> > > > For CPU path, there is no need to have the keys set for XCBC and
> > > > CMAC cases.
> > > >
> > > > Fixes: 010230a1543b ("crypto/aesni_mb: support Chacha20-Poly1305")
> > > > Fixes: b0a37e8cd2ac ("crypto/ipsec_mb: fix cipher key setting")
> > > > Fixes: a2c6d3f34f90 ("crypto/aesni_mb: support CPU crypto")
> > > >
> > > > Signed-off-by: Ciara Power <ciara.power@intel.com>
> > > > ---
> > > > Cc: pablo.de.lara.guarch@intel.com
> > > > ---
> > > >  drivers/crypto/ipsec_mb/pmd_aesni_mb.c | 14 --------------
> > > >  1 file changed, 14 deletions(-)
> > >
> > > I have already tagged rc1 - is this fixing a regression introduced
> > > in
> > > rc1 itself? If not, how important is it, could it wait for the next release?
> >
> > No, it is fixing an issue that existed before 22.11 itself. I have also sent the fix
> for 21.11 LTS.
> > The bug was reported by an external user as it caused seg faults for their
> algorithm use case, so the sooner the better for fix to be merged.
> > Would it be suitable for merge in rc2?
> 
> I am not planning an rc2, but if you can confirm you have tested this patch on
> top of rc1 on the affected platform then I can merge it for the final release.

Hi Luca,

Ah no RC2, ok yes I understand.
I have tested with SW PMD autotests and perf tests on 22.11.5-rc1 + patch, all seems ok.
But if you would prefer to wait until next LTS release, that is fine with me - user can get the fix from the mailing list if needed in the meantime.

Thanks,
Ciara

Re: [PATCH 22.11] crypto/ipsec_mb: fix incorrectly setting cipher keys

[Luca Boccassi]

On Wed, 10 Apr 2024 at 09:06, Power, Ciara <ciara.power@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Luca Boccassi <bluca@debian.org>
> > Sent: Monday, April 8, 2024 10:23 AM
> > To: Power, Ciara <ciara.power@intel.com>
> > Cc: stable@dpdk.org; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> > Ji, Kai <kai.ji@intel.com>
> > Subject: Re: [PATCH 22.11] crypto/ipsec_mb: fix incorrectly setting cipher keys
> >
> > On Mon, 8 Apr 2024 at 08:17, Power, Ciara <ciara.power@intel.com> wrote:
> > >
> > > Hi Luca,
> > >
> > > > -----Original Message-----
> > > > From: Luca Boccassi <luca.boccassi@gmail.com>
> > > > Sent: Friday, April 5, 2024 3:44 PM
> > > > To: Power, Ciara <ciara.power@intel.com>
> > > > Cc: stable@dpdk.org; De Lara Guarch, Pablo
> > > > <pablo.de.lara.guarch@intel.com>; Ji, Kai <kai.ji@intel.com>
> > > > Subject: Re: [PATCH 22.11] crypto/ipsec_mb: fix incorrectly setting
> > > > cipher keys
> > > >
> > > > On Fri, 5 Apr 2024 at 11:46, Ciara Power <ciara.power@intel.com> wrote:
> > > > >
> > > > > The encryption and decryption keys were incorrectly being reset
> > > > > based on authentication algorithm after already being set earlier
> > > > > in the code based on cipher algorithm.
> > > > > In cases when 3DES was used, the keys were being incorrectly
> > > > > overwritten.
> > > > >
> > > > > For CPU path, there is no need to have the keys set for XCBC and
> > > > > CMAC cases.
> > > > >
> > > > > Fixes: 010230a1543b ("crypto/aesni_mb: support Chacha20-Poly1305")
> > > > > Fixes: b0a37e8cd2ac ("crypto/ipsec_mb: fix cipher key setting")
> > > > > Fixes: a2c6d3f34f90 ("crypto/aesni_mb: support CPU crypto")
> > > > >
> > > > > Signed-off-by: Ciara Power <ciara.power@intel.com>
> > > > > ---
> > > > > Cc: pablo.de.lara.guarch@intel.com
> > > > > ---
> > > > >  drivers/crypto/ipsec_mb/pmd_aesni_mb.c | 14 --------------
> > > > >  1 file changed, 14 deletions(-)
> > > >
> > > > I have already tagged rc1 - is this fixing a regression introduced
> > > > in
> > > > rc1 itself? If not, how important is it, could it wait for the next release?
> > >
> > > No, it is fixing an issue that existed before 22.11 itself. I have also sent the fix
> > for 21.11 LTS.
> > > The bug was reported by an external user as it caused seg faults for their
> > algorithm use case, so the sooner the better for fix to be merged.
> > > Would it be suitable for merge in rc2?
> >
> > I am not planning an rc2, but if you can confirm you have tested this patch on
> > top of rc1 on the affected platform then I can merge it for the final release.
>
> Hi Luca,
>
> Ah no RC2, ok yes I understand.
> I have tested with SW PMD autotests and perf tests on 22.11.5-rc1 + patch, all seems ok.
> But if you would prefer to wait until next LTS release, that is fine with me - user can get the fix from the mailing list if needed in the meantime.

That's ok given you have tested I'll push it, thanks.