Re: [PATCH] crypto/qat: fix docsis segmentation fault

Re: [PATCH] crypto/qat: fix docsis segmentation fault

[Troy, Rebecca]

> 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.

Re: [PATCH] crypto/qat: fix docsis segmentation fault

[David Marchand]

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

RE: [PATCH] crypto/qat: fix docsis segmentation fault

[Zhang, Roy Fan]

> -----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>

Re: [PATCH] crypto/qat: fix docsis segmentation fault

[David Marchand]

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

[PATCH] crypto/qat: fix docsis segmentation fault

[Rebecca Troy]

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