From: Damian Muszynski <damian.muszynski@intel.com>
To: Herbert Xu <herbert@gondor.apana.org.au>
Cc: <linux-crypto@vger.kernel.org>, <qat-linux@intel.com>,
Giovanni Cabiddu <giovanni.cabiddu@intel.com>
Subject: Re: [PATCH] crypto: qat - Fix ADF_DEV_RESET_SYNC memory leak
Date: Wed, 8 May 2024 12:23:50 +0200 [thread overview]
Message-ID: <ZjtStuIt2COdz5Dp@dmuszyns-mobl.ger.corp.intel.com> (raw)
In-Reply-To: <Zjs6VxtkL8QLtHIH@gondor.apana.org.au>
Hi Herbert,
Thanks for your vigilance. I based this fix on the description of
completion_done() which can be misunderstood as can be seen.
--
Damian
On 2024-05-08 at 16:39:51 +0800, Herbert Xu wrote:
> On Fri, Feb 09, 2024 at 01:43:42PM +0100, Damian Muszynski wrote:
> >
> > @@ -146,11 +147,19 @@ static void adf_device_reset_worker(struct work_struct *work)
> > adf_dev_restarted_notify(accel_dev);
> > clear_bit(ADF_STATUS_RESTARTING, &accel_dev->status);
> >
> > - /* The dev is back alive. Notify the caller if in sync mode */
> > - if (reset_data->mode == ADF_DEV_RESET_SYNC)
> > - complete(&reset_data->compl);
> > - else
> > + /*
> > + * The dev is back alive. Notify the caller if in sync mode
> > + *
> > + * If device restart will take a more time than expected,
> > + * the schedule_reset() function can timeout and exit. This can be
> > + * detected by calling the completion_done() function. In this case
> > + * the reset_data structure needs to be freed here.
> > + */
> > + if (reset_data->mode == ADF_DEV_RESET_ASYNC ||
> > + completion_done(&reset_data->compl))
> > kfree(reset_data);
> > + else
> > + complete(&reset_data->compl);
>
> This doesn't work because until you call complete, completion_done
> will always return false. IOW we now have a memory leak instead of
> a UAF.
>
> ---8<---
> Using completion_done to determine whether the caller has gone
> away only works after a complete call. Furthermore it's still
> possible that the caller has not yet called wait_for_completion,
> resulting in another potential UAF.
>
> Fix this by making the caller use cancel_work_sync and then freeing
> the memory safely.
>
> Fixes: 7d42e097607c ("crypto: qat - resolve race condition during AER recovery")
> Cc: <stable@vger.kernel.org> #6.8+
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> diff --git a/drivers/crypto/intel/qat/qat_common/adf_aer.c b/drivers/crypto/intel/qat/qat_common/adf_aer.c
> index 9da2278bd5b7..04260f61d042 100644
> --- a/drivers/crypto/intel/qat/qat_common/adf_aer.c
> +++ b/drivers/crypto/intel/qat/qat_common/adf_aer.c
> @@ -130,8 +130,7 @@ static void adf_device_reset_worker(struct work_struct *work)
> if (adf_dev_restart(accel_dev)) {
> /* The device hanged and we can't restart it so stop here */
> dev_err(&GET_DEV(accel_dev), "Restart device failed\n");
> - if (reset_data->mode == ADF_DEV_RESET_ASYNC ||
> - completion_done(&reset_data->compl))
> + if (reset_data->mode == ADF_DEV_RESET_ASYNC)
> kfree(reset_data);
> WARN(1, "QAT: device restart failed. Device is unusable\n");
> return;
> @@ -147,16 +146,8 @@ static void adf_device_reset_worker(struct work_struct *work)
> adf_dev_restarted_notify(accel_dev);
> clear_bit(ADF_STATUS_RESTARTING, &accel_dev->status);
>
> - /*
> - * The dev is back alive. Notify the caller if in sync mode
> - *
> - * If device restart will take a more time than expected,
> - * the schedule_reset() function can timeout and exit. This can be
> - * detected by calling the completion_done() function. In this case
> - * the reset_data structure needs to be freed here.
> - */
> - if (reset_data->mode == ADF_DEV_RESET_ASYNC ||
> - completion_done(&reset_data->compl))
> + /* The dev is back alive. Notify the caller if in sync mode */
> + if (reset_data->mode == ADF_DEV_RESET_ASYNC)
> kfree(reset_data);
> else
> complete(&reset_data->compl);
> @@ -191,10 +182,10 @@ static int adf_dev_aer_schedule_reset(struct adf_accel_dev *accel_dev,
> if (!timeout) {
> dev_err(&GET_DEV(accel_dev),
> "Reset device timeout expired\n");
> + cancel_work_sync(&reset_data->reset_work);
> ret = -EFAULT;
> - } else {
> - kfree(reset_data);
> }
> + kfree(reset_data);
> return ret;
> }
> return 0;
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
next prev parent reply other threads:[~2024-05-08 10:24 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-09 12:43 [PATCH] crypto: qat - resolve race condition during AER recovery Damian Muszynski
2024-02-17 1:15 ` Herbert Xu
2024-05-08 8:39 ` [PATCH] crypto: qat - Fix ADF_DEV_RESET_SYNC memory leak Herbert Xu
2024-05-08 10:23 ` Damian Muszynski [this message]
2024-05-16 21:16 ` Cabiddu, Giovanni
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=ZjtStuIt2COdz5Dp@dmuszyns-mobl.ger.corp.intel.com \
--to=damian.muszynski@intel.com \
--cc=giovanni.cabiddu@intel.com \
--cc=herbert@gondor.apana.org.au \
--cc=linux-crypto@vger.kernel.org \
--cc=qat-linux@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 read-only IMAP folder(s) and NNTP newsgroup(s).