[PATCH] rapidio: fix rio_dma_transfer error handling

[PATCH] rapidio: fix rio_dma_transfer error handling

[Ioan Nicu]

Some of the mport_dma_req structure members were initialized late
inside the do_dma_request() function, just before submitting the
request to the dma engine. But we have some error branches before
that. In case of such an error, the code would return on the error
path and trigger the calling of dma_req_free() with a req structure
which is not completely initialized. This causes a NULL pointer
dereference in dma_req_free().

This patch fixes these error branches by making sure that all
necessary mport_dma_req structure members are initialized in
rio_dma_transfer() immediately after the request structure gets
allocated.

Signed-off-by: Ioan Nicu <ioan.nicu.ext@nokia.com>
---
 drivers/rapidio/devices/rio_mport_cdev.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/rapidio/devices/rio_mport_cdev.c b/drivers/rapidio/devices/rio_mport_cdev.c
index 9d27016c899e..0434ab7b6497 100644
--- a/drivers/rapidio/devices/rio_mport_cdev.c
+++ b/drivers/rapidio/devices/rio_mport_cdev.c
@@ -740,10 +740,7 @@ static int do_dma_request(struct mport_dma_req *req,
 	tx->callback = dma_xfer_callback;
 	tx->callback_param = req;
 
-	req->dmach = chan;
-	req->sync = sync;
 	req->status = DMA_IN_PROGRESS;
-	init_completion(&req->req_comp);
 	kref_get(&req->refcount);
 
 	cookie = dmaengine_submit(tx);
@@ -831,13 +828,20 @@ rio_dma_transfer(struct file *filp, u32 transfer_mode,
 	if (!req)
 		return -ENOMEM;
 
-	kref_init(&req->refcount);
-
 	ret = get_dma_channel(priv);
 	if (ret) {
 		kfree(req);
 		return ret;
 	}
+	chan = priv->dmach;
+
+	kref_init(&req->refcount);
+	init_completion(&req->req_comp);
+	req->dir = dir;
+	req->filp = filp;
+	req->priv = priv;
+	req->dmach = chan;
+	req->sync = sync;
 
 	/*
 	 * If parameter loc_addr != NULL, we are transferring data from/to
@@ -925,11 +929,6 @@ rio_dma_transfer(struct file *filp, u32 transfer_mode,
 				xfer->offset, xfer->length);
 	}
 
-	req->dir = dir;
-	req->filp = filp;
-	req->priv = priv;
-	chan = priv->dmach;
-
 	nents = dma_map_sg(chan->device->dev,
 			   req->sgt.sgl, req->sgt.nents, dir);
 	if (nents == 0) {
-- 
2.16.3

Re: [PATCH] rapidio: fix rio_dma_transfer error handling

[Alexander Sverdlin]

On 12/04/18 17:06, Ioan Nicu wrote:
> Some of the mport_dma_req structure members were initialized late
> inside the do_dma_request() function, just before submitting the
> request to the dma engine. But we have some error branches before
> that. In case of such an error, the code would return on the error
> path and trigger the calling of dma_req_free() with a req structure
> which is not completely initialized. This causes a NULL pointer
> dereference in dma_req_free().
> 
> This patch fixes these error branches by making sure that all
> necessary mport_dma_req structure members are initialized in
> rio_dma_transfer() immediately after the request structure gets
> allocated.
> 
> Signed-off-by: Ioan Nicu <ioan.nicu.ext@nokia.com>

Tested-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>

> ---
>  drivers/rapidio/devices/rio_mport_cdev.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/rapidio/devices/rio_mport_cdev.c b/drivers/rapidio/devices/rio_mport_cdev.c
> index 9d27016c899e..0434ab7b6497 100644
> --- a/drivers/rapidio/devices/rio_mport_cdev.c
> +++ b/drivers/rapidio/devices/rio_mport_cdev.c
> @@ -740,10 +740,7 @@ static int do_dma_request(struct mport_dma_req *req,
>  	tx->callback = dma_xfer_callback;
>  	tx->callback_param = req;
>  
> -	req->dmach = chan;
> -	req->sync = sync;
>  	req->status = DMA_IN_PROGRESS;
> -	init_completion(&req->req_comp);
>  	kref_get(&req->refcount);
>  
>  	cookie = dmaengine_submit(tx);
> @@ -831,13 +828,20 @@ rio_dma_transfer(struct file *filp, u32 transfer_mode,
>  	if (!req)
>  		return -ENOMEM;
>  
> -	kref_init(&req->refcount);
> -
>  	ret = get_dma_channel(priv);
>  	if (ret) {
>  		kfree(req);
>  		return ret;
>  	}
> +	chan = priv->dmach;
> +
> +	kref_init(&req->refcount);
> +	init_completion(&req->req_comp);
> +	req->dir = dir;
> +	req->filp = filp;
> +	req->priv = priv;
> +	req->dmach = chan;
> +	req->sync = sync;
>  
>  	/*
>  	 * If parameter loc_addr != NULL, we are transferring data from/to
> @@ -925,11 +929,6 @@ rio_dma_transfer(struct file *filp, u32 transfer_mode,
>  				xfer->offset, xfer->length);
>  	}
>  
> -	req->dir = dir;
> -	req->filp = filp;
> -	req->priv = priv;
> -	chan = priv->dmach;
> -
>  	nents = dma_map_sg(chan->device->dev,
>  			   req->sgt.sgl, req->sgt.nents, dir);
>  	if (nents == 0) {

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH] rapidio: fix rio_dma_transfer error handling

[Alexandre Bounine]

On 2018-04-12 11:06 AM, Ioan Nicu wrote:
> Some of the mport_dma_req structure members were initialized late
> inside the do_dma_request() function, just before submitting the
> request to the dma engine. But we have some error branches before
> that. In case of such an error, the code would return on the error
> path and trigger the calling of dma_req_free() with a req structure
> which is not completely initialized. This causes a NULL pointer
> dereference in dma_req_free().
> 
> This patch fixes these error branches by making sure that all
> necessary mport_dma_req structure members are initialized in
> rio_dma_transfer() immediately after the request structure gets
> allocated.
> 
> Signed-off-by: Ioan Nicu <ioan.nicu.ext@nokia.com>
> ---
>   drivers/rapidio/devices/rio_mport_cdev.c | 19 +++++++++----------
>   1 file changed, 9 insertions(+), 10 deletions(-)
> 
Acked-by: Alexandre Bounine <alex.bou9@gmail.com>

Re: [PATCH] rapidio: fix rio_dma_transfer error handling

[Andrew Morton]

On Thu, 12 Apr 2018 17:06:05 +0200 Ioan Nicu <ioan.nicu.ext@nokia.com> wrote:

> Some of the mport_dma_req structure members were initialized late
> inside the do_dma_request() function, just before submitting the
> request to the dma engine. But we have some error branches before
> that. In case of such an error, the code would return on the error
> path and trigger the calling of dma_req_free() with a req structure
> which is not completely initialized. This causes a NULL pointer
> dereference in dma_req_free().
> 
> This patch fixes these error branches by making sure that all
> necessary mport_dma_req structure members are initialized in
> rio_dma_transfer() immediately after the request structure gets
> allocated.

This sounds like something which someone has actually triggered in a
real-world situation.  So I added a cc:stable.  Please let me know if
that was inappropriate.

And please remember to always include all information regarding
end-user impact when fixing bugs.

Re: [PATCH] rapidio: fix rio_dma_transfer error handling

[Alexandre Bounine]

On 2018-04-12 05:28 PM, Andrew Morton wrote:
> On Thu, 12 Apr 2018 17:06:05 +0200 Ioan Nicu <ioan.nicu.ext@nokia.com> wrote:
> 
>> Some of the mport_dma_req structure members were initialized late
>> inside the do_dma_request() function, just before submitting the
>> request to the dma engine. But we have some error branches before
>> that. In case of such an error, the code would return on the error
>> path and trigger the calling of dma_req_free() with a req structure
>> which is not completely initialized. This causes a NULL pointer
>> dereference in dma_req_free().
>>
>> This patch fixes these error branches by making sure that all
>> necessary mport_dma_req structure members are initialized in
>> rio_dma_transfer() immediately after the request structure gets
>> allocated.
> 
> This sounds like something which someone has actually triggered in a
> real-world situation.  So I added a cc:stable.  Please let me know if
> that was inappropriate.
> 
> And please remember to always include all information regarding
> end-user impact when fixing bugs.
> 
This bug fix is applicable to versions starting from v4.6

Re: [PATCH] rapidio: fix rio_dma_transfer error handling

[Ioan Nicu]

Hi,

On Thu, Apr 12, 2018 at 07:44:01PM -0400, Alexandre Bounine wrote:
> 
> On 2018-04-12 05:28 PM, Andrew Morton wrote:
> > On Thu, 12 Apr 2018 17:06:05 +0200 Ioan Nicu <ioan.nicu.ext@nokia.com> wrote:
> > 
> > > Some of the mport_dma_req structure members were initialized late
> > > inside the do_dma_request() function, just before submitting the
> > > request to the dma engine. But we have some error branches before
> > > that. In case of such an error, the code would return on the error
> > > path and trigger the calling of dma_req_free() with a req structure
> > > which is not completely initialized. This causes a NULL pointer
> > > dereference in dma_req_free().
> > > 
> > > This patch fixes these error branches by making sure that all
> > > necessary mport_dma_req structure members are initialized in
> > > rio_dma_transfer() immediately after the request structure gets
> > > allocated.
> > 
> > This sounds like something which someone has actually triggered in a
> > real-world situation.  So I added a cc:stable.  Please let me know if
> > that was inappropriate.
> > 
> > And please remember to always include all information regarding
> > end-user impact when fixing bugs.
> > 
> This bug fix is applicable to versions starting from v4.6

Actually, this is something I broke with my previous patch where I added a
kref to the mport_dma_req structure. Before this patch, all the error paths
were doing kfree(req) instead of kref_put(&req->refcount, dma_req_free).

Now that dma_req_free() is called, it dereferences req->dmach, which is
initialized late in do_dma_request(), so dma_req_free() could be called
with a NULL req->dmach in some cases.

Sorry if I did not make this clear enough in the description.

Regards,
Ioan

Re: [PATCH] rapidio: fix rio_dma_transfer error handling

[Andrew Morton]

On Fri, 13 Apr 2018 09:09:18 +0200 Ioan Nicu <ioan.nicu.ext@nokia.com> wrote:

> > > And please remember to always include all information regarding
> > > end-user impact when fixing bugs.
> > > 
> > This bug fix is applicable to versions starting from v4.6
> 
> Actually, this is something I broke with my previous patch where I added a
> kref to the mport_dma_req structure. Before this patch, all the error paths
> were doing kfree(req) instead of kref_put(&req->refcount, dma_req_free).
> 
> Now that dma_req_free() is called, it dereferences req->dmach, which is
> initialized late in do_dma_request(), so dma_req_free() could be called
> with a NULL req->dmach in some cases.
> 
> Sorry if I did not make this clear enough in the description.

I added

Fixes: bbd876adb8c72 ("rapidio: use a reference count for struct mport_dma_req")

(correct?) and removed cc:stable.