From: Sudeep Dutt <sudeep.dutt@intel.com>
To: Wenwen Wang <wang6495@umn.edu>
Cc: Sudeep Dutt <sudeep.dutt@intel.com>, Kangjie Lu <kjlu@umn.edu>,
Ashutosh Dixit <ashutosh.dixit@intel.com>,
Arnd Bergmann <arnd@arndb.de>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] misc: mic: fix a DMA pool free failure
Date: Wed, 02 Jan 2019 17:50:47 -0800 [thread overview]
Message-ID: <1546480247.23374.14.camel@intel.com> (raw)
In-Reply-To: <1543936604-6194-1-git-send-email-wang6495@umn.edu>
On Tue, 2018-12-04 at 09:16 -0600, Wenwen Wang wrote:
> In _scif_prog_signal(), a DMA pool is allocated if the MIC Coprocessor is
> not X100, i.e., the boolean variable 'x100' is false. This DMA pool will be
> freed eventually through the callback function scif_prog_signal_cb() with
> the parameter of 'status', which actually points to the start of DMA pool.
> Specifically, in scif_prog_signal_cb(), the 'ep' field and the
> 'src_dma_addr' field of 'status' are used to free the DMA pool by invoking
> dma_pool_free(). Given that 'status' points to the start address of the DMA
> pool, both 'status->ep' and 'status->src_dma_addr' are in the DMA pool. And
> so, the device has the permission to access them. Even worse, a malicious
> device can modify them. As a result, dma_pool_free() will not succeed.
>
> To avoid the above issue, this patch introduces a new data structure, i.e.,
> scif_cb_arg, to store the arguments required by the call back function. A
> variable 'cb_arg' is allocated in _scif_prog_signal() to pass the
> arguments. 'cb_arg' will be freed after dma_pool_free() in
> scif_prog_signal_cb().
>
Thanks for incorporating the previous feedback Wenwen.
Reviewed-by: Sudeep Dutt <sudeep.dutt@intel.com>
> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
> ---
> drivers/misc/mic/scif/scif_fence.c | 22 +++++++++++++++++-----
> drivers/misc/mic/scif/scif_rma.h | 13 +++++++++++++
> 2 files changed, 30 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/misc/mic/scif/scif_fence.c b/drivers/misc/mic/scif/scif_fence.c
> index 7bb929f..2e7ce6a 100644
> --- a/drivers/misc/mic/scif/scif_fence.c
> +++ b/drivers/misc/mic/scif/scif_fence.c
> @@ -195,10 +195,11 @@ static inline void *scif_get_local_va(off_t off, struct scif_window *window)
>
> static void scif_prog_signal_cb(void *arg)
> {
> - struct scif_status *status = arg;
> + struct scif_cb_arg *cb_arg = arg;
>
> - dma_pool_free(status->ep->remote_dev->signal_pool, status,
> - status->src_dma_addr);
> + dma_pool_free(cb_arg->ep->remote_dev->signal_pool, cb_arg->status,
> + cb_arg->src_dma_addr);
> + kfree(cb_arg);
> }
>
> static int _scif_prog_signal(scif_epd_t epd, dma_addr_t dst, u64 val)
> @@ -209,6 +210,7 @@ static int _scif_prog_signal(scif_epd_t epd, dma_addr_t dst, u64 val)
> bool x100 = !is_dma_copy_aligned(chan->device, 1, 1, 1);
> struct dma_async_tx_descriptor *tx;
> struct scif_status *status = NULL;
> + struct scif_cb_arg *cb_arg = NULL;
> dma_addr_t src;
> dma_cookie_t cookie;
> int err;
> @@ -257,8 +259,16 @@ static int _scif_prog_signal(scif_epd_t epd, dma_addr_t dst, u64 val)
> goto dma_fail;
> }
> if (!x100) {
> + cb_arg = kmalloc(sizeof(*cb_arg), GFP_KERNEL);
> + if (!cb_arg) {
> + err = -ENOMEM;
> + goto dma_fail;
> + }
> + cb_arg->src_dma_addr = src;
> + cb_arg->status = status;
> + cb_arg->ep = ep;
> tx->callback = scif_prog_signal_cb;
> - tx->callback_param = status;
> + tx->callback_param = cb_arg;
> }
> cookie = tx->tx_submit(tx);
> if (dma_submit_error(cookie)) {
> @@ -270,9 +280,11 @@ static int _scif_prog_signal(scif_epd_t epd, dma_addr_t dst, u64 val)
> dma_async_issue_pending(chan);
> return 0;
> dma_fail:
> - if (!x100)
> + if (!x100) {
> dma_pool_free(ep->remote_dev->signal_pool, status,
> src - offsetof(struct scif_status, val));
> + kfree(cb_arg);
> + }
> alloc_fail:
> return err;
> }
> diff --git a/drivers/misc/mic/scif/scif_rma.h b/drivers/misc/mic/scif/scif_rma.h
> index fa67222..84af303 100644
> --- a/drivers/misc/mic/scif/scif_rma.h
> +++ b/drivers/misc/mic/scif/scif_rma.h
> @@ -206,6 +206,19 @@ struct scif_status {
> };
>
> /*
> + * struct scif_cb_arg - Stores the argument of the callback func
> + *
> + * @src_dma_addr: Source buffer DMA address
> + * @status: DMA status
> + * @ep: SCIF endpoint
> + */
> +struct scif_cb_arg {
> + dma_addr_t src_dma_addr;
> + struct scif_status *status;
> + struct scif_endpt *ep;
> +};
> +
> +/*
> * struct scif_window - Registration Window for Self and Remote
> *
> * @nr_pages: Number of pages which is defined as a s64 instead of an int
prev parent reply other threads:[~2019-01-03 1:32 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-04 15:16 [PATCH v2] misc: mic: fix a DMA pool free failure Wenwen Wang
2019-01-03 1:50 ` Sudeep Dutt [this message]
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=1546480247.23374.14.camel@intel.com \
--to=sudeep.dutt@intel.com \
--cc=arnd@arndb.de \
--cc=ashutosh.dixit@intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=kjlu@umn.edu \
--cc=linux-kernel@vger.kernel.org \
--cc=wang6495@umn.edu \
/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).