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

[David Marchand <david.marchand@redhat.com>]

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