From: David Marchand <david.marchand@redhat.com>
To: "Troy, Rebecca" <rebecca.troy@intel.com>
Cc: "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>,
"dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [PATCH] crypto/qat: fix docsis segmentation fault
Date: Tue, 28 Jun 2022 13:22:29 +0200 [thread overview]
Message-ID: <CAJFAV8xLOA4dW0_RXBZdc1nuoinaRaD=a-x5=jaxyrfJ2iJHvA@mail.gmail.com> (raw)
In-Reply-To: <BYAPR11MB3846CD004634E1908F3E87B580B89@BYAPR11MB3846.namprd11.prod.outlook.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
next prev parent reply other threads:[~2022-06-28 11:22 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-28 10:32 Troy, Rebecca
2022-06-28 11:22 ` David Marchand [this message]
-- strict thread matches above, loose matches on Subject: below --
2022-06-27 16:43 Rebecca Troy
2022-06-28 7:31 ` David Marchand
2022-06-29 8:09 ` Zhang, Roy Fan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAJFAV8xLOA4dW0_RXBZdc1nuoinaRaD=a-x5=jaxyrfJ2iJHvA@mail.gmail.com' \
--to=david.marchand@redhat.com \
--cc=arkadiuszx.kusztal@intel.com \
--cc=dev@dpdk.org \
--cc=fiona.trahe@intel.com \
--cc=rebecca.troy@intel.com \
--cc=roy.fan.zhang@intel.com \
--cc=tomaszx.jozwiak@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).