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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 04 Nov 2018 18:23:33 -0800
From:   Sudeep Dutt <sudeep.dutt@...el.com>
To:     Wenwen Wang <wang6495@....edu>
Cc:     Sudeep Dutt <sudeep.dutt@...el.com>, Kangjie Lu <kjlu@....edu>,
        Ashutosh Dixit <ashutosh.dixit@...el.com>,
        Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: 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 <wang6495@....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