DPDK patches and discussions
 help / color / mirror / Atom feed
* [PATCH] crypto/qat: fix docsis segmentation fault
@ 2022-06-27 16:43 Rebecca Troy
  2022-06-28  7:31 ` David Marchand
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Rebecca Troy @ 2022-06-27 16:43 UTC (permalink / raw)
  To: Fan Zhang, Arkadiusz Kusztal, Fiona Trahe, Tomasz Jozwiak
  Cc: dev, Rebecca Troy, stable

Currently if AES or DES algorithms fail for Docsis test suite,
a segmentation fault occurs when cryptodev_qat_autotest is ran.

This is due to a duplicate call of EVP_CIPHER_CTX_free for the
session context. Ctx is freed firstly in the bpi_cipher_ctx_init
function and then again at the end of qat_sym_session_configure_cipher
function.

This commit fixes this bug by removing the first instance
of EVP_CIPHER_CTX_free, leaving just the dedicated function in
the upper level to free the ctx.

Fixes: 98f060891615 ("crypto/qat: add symmetric session file")
Cc: fiona.trahe@intel.com
Cc: stable@dpdk.org

Signed-off-by: Rebecca Troy <rebecca.troy@intel.com>
---
 drivers/crypto/qat/qat_sym_session.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/crypto/qat/qat_sym_session.c b/drivers/crypto/qat/qat_sym_session.c
index e40c042ba9..568792b753 100644
--- a/drivers/crypto/qat/qat_sym_session.c
+++ b/drivers/crypto/qat/qat_sym_session.c
@@ -111,12 +111,12 @@ bpi_cipher_ctx_init(enum rte_crypto_cipher_algorithm cryptodev_algo,
 		const uint8_t *key, uint16_t key_length, void **ctx)
 {
 	const EVP_CIPHER *algo = NULL;
-	int ret;
+	int ret = 0;
 	*ctx = EVP_CIPHER_CTX_new();
 
 	if (*ctx == NULL) {
 		ret = -ENOMEM;
-		goto ctx_init_err;
+		return ret;
 	}
 
 	if (cryptodev_algo == RTE_CRYPTO_CIPHER_DES_DOCSISBPI)
@@ -130,14 +130,9 @@ bpi_cipher_ctx_init(enum rte_crypto_cipher_algorithm cryptodev_algo,
 	/* IV will be ECB encrypted whether direction is encrypt or decrypt*/
 	if (EVP_EncryptInit_ex(*ctx, algo, NULL, key, 0) != 1) {
 		ret = -EINVAL;
-		goto ctx_init_err;
+		return ret;
 	}
 
-	return 0;
-
-ctx_init_err:
-	if (*ctx != NULL)
-		EVP_CIPHER_CTX_free(*ctx);
 	return ret;
 }
 
-- 
2.34.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] crypto/qat: fix docsis segmentation fault
  2022-06-27 16:43 [PATCH] crypto/qat: fix docsis segmentation fault Rebecca Troy
@ 2022-06-28  7:31 ` David Marchand
  2022-06-29  8:09 ` Zhang, Roy Fan
  2022-06-29 16:10 ` [PATCH v2] " Rebecca Troy
  2 siblings, 0 replies; 7+ messages in thread
From: David Marchand @ 2022-06-28  7:31 UTC (permalink / raw)
  To: Rebecca Troy
  Cc: Fan Zhang, Arkadiusz Kusztal, Fiona Trahe, Tomasz Jozwiak, dev,
	dpdk stable

On Mon, Jun 27, 2022 at 6:45 PM Rebecca Troy <rebecca.troy@intel.com> wrote:
>
> Currently if AES or DES algorithms fail for Docsis test suite,
> a segmentation fault occurs when cryptodev_qat_autotest is ran.
>
> This is due to a duplicate call of EVP_CIPHER_CTX_free for the
> session context. Ctx is freed firstly in the bpi_cipher_ctx_init
> function and then again at the end of qat_sym_session_configure_cipher
> function.
>
> This commit fixes this bug by removing the first instance
> of EVP_CIPHER_CTX_free, leaving just the dedicated function in
> the upper level to free the ctx.

This is awkward.
This helper should let *ctx alone until everything succeeded.

-- 
David Marchand


^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH] crypto/qat: fix docsis segmentation fault
  2022-06-27 16:43 [PATCH] crypto/qat: fix docsis segmentation fault Rebecca Troy
  2022-06-28  7:31 ` David Marchand
@ 2022-06-29  8:09 ` Zhang, Roy Fan
  2022-06-29 16:10 ` [PATCH v2] " Rebecca Troy
  2 siblings, 0 replies; 7+ messages in thread
From: Zhang, Roy Fan @ 2022-06-29  8:09 UTC (permalink / raw)
  To: Troy, Rebecca, Kusztal, ArkadiuszX, Trahe, Fiona, Tomasz Jozwiak
  Cc: dev, stable

> -----Original Message-----
> From: Troy, Rebecca <rebecca.troy@intel.com>
> Sent: Monday, June 27, 2022 5:44 PM
> To: Zhang, Roy Fan <roy.fan.zhang@intel.com>; Kusztal, ArkadiuszX
> <arkadiuszx.kusztal@intel.com>; Trahe, Fiona <fiona.trahe@intel.com>; Tomasz
> Jozwiak <tomaszx.jozwiak@intel.com>
> Cc: dev@dpdk.org; Troy, Rebecca <rebecca.troy@intel.com>; stable@dpdk.org
> Subject: [PATCH] crypto/qat: fix docsis segmentation fault
> 
> Currently if AES or DES algorithms fail for Docsis test suite,
> a segmentation fault occurs when cryptodev_qat_autotest is ran.
> 
> This is due to a duplicate call of EVP_CIPHER_CTX_free for the
> session context. Ctx is freed firstly in the bpi_cipher_ctx_init
> function and then again at the end of qat_sym_session_configure_cipher
> function.
> 
> This commit fixes this bug by removing the first instance
> of EVP_CIPHER_CTX_free, leaving just the dedicated function in
> the upper level to free the ctx.
> 
> Fixes: 98f060891615 ("crypto/qat: add symmetric session file")
> Cc: fiona.trahe@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Rebecca Troy <rebecca.troy@intel.com>
> ---

Acked-by: Fan Zhang <roy.fan.zhang@intel.com>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2] crypto/qat: fix docsis segmentation fault
  2022-06-27 16:43 [PATCH] crypto/qat: fix docsis segmentation fault Rebecca Troy
  2022-06-28  7:31 ` David Marchand
  2022-06-29  8:09 ` Zhang, Roy Fan
@ 2022-06-29 16:10 ` Rebecca Troy
  2022-06-30  4:29   ` [EXT] " Akhil Goyal
  2 siblings, 1 reply; 7+ messages in thread
From: Rebecca Troy @ 2022-06-29 16:10 UTC (permalink / raw)
  To: Fan Zhang; +Cc: dev, Rebecca Troy, fiona.trahe, stable

Currently if AES or DES algorithms fail for Docsis test suite,
a segmentation fault occurs when cryptodev_qat_autotest is ran.

This is due to a duplicate call of EVP_CIPHER_CTX_free for the
session context. Ctx is freed firstly in the bpi_cipher_ctx_init
function and then again at the end of qat_sym_session_configure_cipher
function.

This commit fixes this bug by removing the first instance
of EVP_CIPHER_CTX_free, leaving just the dedicated function in
the upper level to free the ctx.

Fixes: 98f0608 ("crypto/qat: add symmetric session file")
Cc: fiona.trahe@intel.com
Cc: stable@dpdk.org

Signed-off-by: Rebecca Troy <rebecca.troy@intel.com>
Acked-by: Fan Zhang <roy.fan.zhang@intel.com>

---
V2:
 - Changed the bug fix to set *ctx to NULL after the first
instance of EVP_CIPHER_CTX_free, rather than removing it.
This will ensure NULL checks are met appropriately in further
EVP_CIPHER_CTX_free calls, avoiding segmentation fault without
relying solely on the caller to free the *ctx.
.
---
 drivers/crypto/qat/qat_sym_session.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/qat/qat_sym_session.c b/drivers/crypto/qat/qat_sym_session.c
index e40c042ba9..b30396487e 100644
--- a/drivers/crypto/qat/qat_sym_session.c
+++ b/drivers/crypto/qat/qat_sym_session.c
@@ -136,8 +136,10 @@ bpi_cipher_ctx_init(enum rte_crypto_cipher_algorithm cryptodev_algo,
 	return 0;
 
 ctx_init_err:
-	if (*ctx != NULL)
+	if (*ctx != NULL) {
 		EVP_CIPHER_CTX_free(*ctx);
+		*ctx = NULL;
+	}
 	return ret;
 }
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [EXT] [PATCH v2] crypto/qat: fix docsis segmentation fault
  2022-06-29 16:10 ` [PATCH v2] " Rebecca Troy
@ 2022-06-30  4:29   ` Akhil Goyal
  0 siblings, 0 replies; 7+ messages in thread
From: Akhil Goyal @ 2022-06-30  4:29 UTC (permalink / raw)
  To: Rebecca Troy, Fan Zhang; +Cc: dev, fiona.trahe, stable

> Currently if AES or DES algorithms fail for Docsis test suite,
> a segmentation fault occurs when cryptodev_qat_autotest is ran.
> 
> This is due to a duplicate call of EVP_CIPHER_CTX_free for the
> session context. Ctx is freed firstly in the bpi_cipher_ctx_init
> function and then again at the end of qat_sym_session_configure_cipher
> function.
> 
> This commit fixes this bug by removing the first instance
> of EVP_CIPHER_CTX_free, leaving just the dedicated function in
> the upper level to free the ctx.
> 
> Fixes: 98f0608 ("crypto/qat: add symmetric session file")
> Cc: fiona.trahe@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Rebecca Troy <rebecca.troy@intel.com>
> Acked-by: Fan Zhang <roy.fan.zhang@intel.com>
Applied to dpdk-next-crypto

Thanks.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] crypto/qat: fix docsis segmentation fault
  2022-06-28 10:32 [PATCH] " Troy, Rebecca
@ 2022-06-28 11:22 ` David Marchand
  0 siblings, 0 replies; 7+ messages in thread
From: David Marchand @ 2022-06-28 11:22 UTC (permalink / raw)
  To: Troy, Rebecca
  Cc: Zhang, Roy Fan, Kusztal, ArkadiuszX, Trahe, Fiona, Tomasz Jozwiak, dev

On Tue, Jun 28, 2022 at 12:32 PM Troy, Rebecca <rebecca.troy@intel.com> wrote:
>
> > On Mon, Jun 27, 2022 at 6:45 PM Rebecca Troy <rebecca.troy@intel.com>
> > wrote:
> > >
> > > Currently if AES or DES algorithms fail for Docsis test suite, a
> > > segmentation fault occurs when cryptodev_qat_autotest is ran.
> > >
> > > This is due to a duplicate call of EVP_CIPHER_CTX_free for the session
> > > context. Ctx is freed firstly in the bpi_cipher_ctx_init function and
> > > then again at the end of qat_sym_session_configure_cipher function.
> > >
> > > This commit fixes this bug by removing the first instance of
> > > EVP_CIPHER_CTX_free, leaving just the dedicated function in the upper
> > > level to free the ctx.
> >
> > This is awkward.
> > This helper should let *ctx alone until everything succeeded.
> >
> > --
> > David Marchand
>
> Hi David,
>
> This bug was found under unusual circumstances. Unit tests failed due to no legacy algorithm support on the system, which caused a segmentation fault rather than the expected 'Tests failed' result.
>
> When these unit tests fail, the current error handling directs that the *ctx be freed twice - once prematurely (which is the instance this patch is removing) and then again after the tests have registered as failed, causing the segmentation fault. I agree that the *ctx shouldn't have been freed prematurely, so this patch fixes the incorrect error handling here by removing that first instance of *ctx freeing.

My point is that bpi_cipher_ctx_init should not return an allocated
object on failure and hope that the caller would free it.
This is counter intuitive.
If someone starts using this helper somewhere else in the code and
does not free the ctx on failure, we will have a leak.


I see two alternatives to fix the issue in a cleaner way:
- either set *ctx to NULL on free,

diff --git a/drivers/crypto/qat/qat_sym_session.c
b/drivers/crypto/qat/qat_sym_session.c
index e40c042ba9..b30396487e 100644
--- a/drivers/crypto/qat/qat_sym_session.c
+++ b/drivers/crypto/qat/qat_sym_session.c
@@ -136,8 +136,10 @@ bpi_cipher_ctx_init(enum
rte_crypto_cipher_algorithm cryptodev_algo,
        return 0;

 ctx_init_err:
-       if (*ctx != NULL)
+       if (*ctx != NULL) {
                EVP_CIPHER_CTX_free(*ctx);
+               *ctx = NULL;
+       }
        return ret;
 }



- or use a temp variable for the allocated cipher ctx object and store
it to *ctx only before returning 0, which was what I meant with my
initial comment.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] crypto/qat: fix docsis segmentation fault
@ 2022-06-28 10:32 Troy, Rebecca
  2022-06-28 11:22 ` David Marchand
  0 siblings, 1 reply; 7+ messages in thread
From: Troy, Rebecca @ 2022-06-28 10:32 UTC (permalink / raw)
  To: Zhang, Roy Fan, Kusztal, ArkadiuszX, Trahe, Fiona,
	Tomasz Jozwiak, david.marchand
  Cc: dev

> On Mon, Jun 27, 2022 at 6:45 PM Rebecca Troy <rebecca.troy@intel.com>
> wrote:
> >
> > Currently if AES or DES algorithms fail for Docsis test suite, a
> > segmentation fault occurs when cryptodev_qat_autotest is ran.
> >
> > This is due to a duplicate call of EVP_CIPHER_CTX_free for the session
> > context. Ctx is freed firstly in the bpi_cipher_ctx_init function and
> > then again at the end of qat_sym_session_configure_cipher function.
> >
> > This commit fixes this bug by removing the first instance of
> > EVP_CIPHER_CTX_free, leaving just the dedicated function in the upper
> > level to free the ctx.
> 
> This is awkward.
> This helper should let *ctx alone until everything succeeded.
> 
> --
> David Marchand

Hi David,

This bug was found under unusual circumstances. Unit tests failed due to no legacy algorithm support on the system, which caused a segmentation fault rather than the expected 'Tests failed' result.

When these unit tests fail, the current error handling directs that the *ctx be freed twice - once prematurely (which is the instance this patch is removing) and then again after the tests have registered as failed, causing the segmentation fault. I agree that the *ctx shouldn't have been freed prematurely, so this patch fixes the incorrect error handling here by removing that first instance of *ctx freeing.

Hope this clears things up.

Rebecca Troy.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-06-30  4:29 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-27 16:43 [PATCH] crypto/qat: fix docsis segmentation fault Rebecca Troy
2022-06-28  7:31 ` David Marchand
2022-06-29  8:09 ` Zhang, Roy Fan
2022-06-29 16:10 ` [PATCH v2] " Rebecca Troy
2022-06-30  4:29   ` [EXT] " Akhil Goyal
2022-06-28 10:32 [PATCH] " Troy, Rebecca
2022-06-28 11:22 ` David Marchand

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).