[PATCH] misc: mic: fix a DMA pool free failure

[PATCH] misc: mic: fix a DMA pool free failure

[Wenwen Wang]

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

Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
 drivers/misc/mic/scif/scif_fence.c | 17 +++++++++++++----
 drivers/misc/mic/scif/scif_rma.h   | 14 ++++++++++++++
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/mic/scif/scif_fence.c b/drivers/misc/mic/scif/scif_fence.c
index cac3bcc..30f7d9b 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,15 @@ static int _scif_prog_signal(scif_epd_t epd, dma_addr_t dst, u64 val)
 		goto dma_fail;
 	}
 	if (!x100) {
+		err = -ENOMEM;
+		cb_arg = kmalloc(sizeof(*cb_arg), GFP_KERNEL);
+		if (!cb_arg)
+			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)) {
diff --git a/drivers/misc/mic/scif/scif_rma.h b/drivers/misc/mic/scif/scif_rma.h
index fa67222..e3b80e1 100644
--- a/drivers/misc/mic/scif/scif_rma.h
+++ b/drivers/misc/mic/scif/scif_rma.h
@@ -206,6 +206,20 @@ 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
-- 
2.7.4

Re: [PATCH] misc: mic: fix a DMA pool free failure

[Sudeep Dutt]

On Thu, 2018-10-18 at 14:46 -0500, 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().
> 
> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
> ---
>  drivers/misc/mic/scif/scif_fence.c | 17 +++++++++++++----
>  drivers/misc/mic/scif/scif_rma.h   | 14 ++++++++++++++
>  2 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/misc/mic/scif/scif_fence.c b/drivers/misc/mic/scif/scif_fence.c
> index cac3bcc..30f7d9b 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,15 @@ static int _scif_prog_signal(scif_epd_t epd, dma_addr_t dst, u64 val)
>  		goto dma_fail;
>  	}
>  	if (!x100) {
> +		err = -ENOMEM;

Should err be set to -ENOMEM only if the cb_arg allocation fails?

> +		cb_arg = kmalloc(sizeof(*cb_arg), GFP_KERNEL);
> +		if (!cb_arg)
> +			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;
>  	}

cb_arg should be freed if there is a dma_submit_error(..) below in the
dma_fail path.

Wenwen, can you please fix these up and resend the patch?

Thanks,
Sudeep Dutt

Re: [PATCH] misc: mic: fix a DMA pool free failure

[Wenwen Wang]

On Sun, Nov 4, 2018 at 8:05 PM Sudeep Dutt <sudeep.dutt@intel.com> wrote:
>
> On Thu, 2018-10-18 at 14:46 -0500, 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().
> >
> > Signed-off-by: Wenwen Wang <wang6495@umn.edu>
> > ---
> >  drivers/misc/mic/scif/scif_fence.c | 17 +++++++++++++----
> >  drivers/misc/mic/scif/scif_rma.h   | 14 ++++++++++++++
> >  2 files changed, 27 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/misc/mic/scif/scif_fence.c b/drivers/misc/mic/scif/scif_fence.c
> > index cac3bcc..30f7d9b 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,15 @@ static int _scif_prog_signal(scif_epd_t epd, dma_addr_t dst, u64 val)
> >               goto dma_fail;
> >       }
> >       if (!x100) {
> > +             err = -ENOMEM;
>
> Should err be set to -ENOMEM only if the cb_arg allocation fails?
>
> > +             cb_arg = kmalloc(sizeof(*cb_arg), GFP_KERNEL);
> > +             if (!cb_arg)
> > +                     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;
> >       }
>
> cb_arg should be freed if there is a dma_submit_error(..) below in the
> dma_fail path.
>
> Wenwen, can you please fix these up and resend the patch?

Sure, will fix the issues and resubmit the patch. Thanks!

Wenwen

[PATCH] misc: mic: fix a DMA pool free failure

[Wenwen Wang]

In _scif_prog_signal(), the boolean variable 'x100' is used to indicate
whether the MIC Coprocessor is X100. If 'x100' is true, the status
descriptor will be used to write the value to the destination. Otherwise, a
DMA pool will be allocated for this purpose. Specifically, if the DMA pool
is allocated successfully, two memory addresses will be returned. One is
for the CPU and the other is for the device to access the DMA pool. The
former is stored to the variable 'status' and the latter is stored to the
variable 'src'. After the allocation, the address in 'src' is saved to
'status->src_dma_addr', which is actually in the DMA pool, and 'src' is
then modified.

Later on, if an error occurs, the execution flow will transfer to the label
'dma_fail', which will check 'x100' and free up the allocated DMA pool if
'x100' is false. The point here is that 'status->src_dma_addr' is used for
freeing up the DMA pool. As mentioned before, 'status->src_dma_addr' is in
the DMA pool. And thus, the device is able to modify this data. This can
potentially cause failures when freeing up the DMA pool because of the
modified device address.

This patch avoids the above issue by using the variable 'src' (with
necessary calculation) to free up the DMA pool.

Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
 drivers/misc/mic/scif/scif_fence.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/mic/scif/scif_fence.c b/drivers/misc/mic/scif/scif_fence.c
index cac3bcc..7bb929f 100644
--- a/drivers/misc/mic/scif/scif_fence.c
+++ b/drivers/misc/mic/scif/scif_fence.c
@@ -272,7 +272,7 @@ static int _scif_prog_signal(scif_epd_t epd, dma_addr_t dst, u64 val)
 dma_fail:
 	if (!x100)
 		dma_pool_free(ep->remote_dev->signal_pool, status,
-			      status->src_dma_addr);
+			      src - offsetof(struct scif_status, val));
 alloc_fail:
 	return err;
 }
-- 
2.7.4