rapidio: fix rio_dma_transfer error handling

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 <[email protected]>

---
 drivers/rapidio/devices/rio_mport_cdev.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

drivers/rapidio/devices/rio_mport_cdev.c +9 -10

index 9d27016c899e..0434ab7b6497 100644
@@ -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) {

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 <[email protected]>

---
  drivers/rapidio/devices/rio_mport_cdev.c | 19 +++++++++----------
  1 file changed, 9 insertions(+), 10 deletions(-)

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.

Quoted 8 lines · On Thu, 12 Apr 2018 17:06:05 +0200 Ioan Nicu <[email protected]> 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().

Quoted 4 lines

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.

Quoted 15 lines · Andrew Morton

Quoted 12 lines · On Thu, 12 Apr 2018 17:06:05 +0200 Ioan Nicu <[email protected]> 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.

Quoted 7 lines

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

Quoted 4 lines

Quoted 3 lines

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.