Re: [PATCH] misc: mic: fix a DMA pool free failure
On Sun, Nov 4, 2018 at 8:05 PM Sudeep Dutt 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
> > ---
> > 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
Re: [PATCH] misc: mic: fix a DMA pool free failure
On Sun, Nov 4, 2018 at 8:05 PM Sudeep Dutt 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
> > ---
> > 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
Re: [PATCH] misc: mic: fix a DMA pool free failure
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
> ---
> 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
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
> ---
> 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
