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

[PATCH v2] 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 | 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
-- 
2.7.4

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

[Sudeep Dutt]

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