[PATCH] usb: cdnsp: fix error handling in cdnsp_mem_init()

[PATCH] usb: cdnsp: fix error handling in cdnsp_mem_init()

[Pawel Laszczak]

This function uses "One Function Cleans up Everything" style and that's
basically impossible to do correctly. It's cleaner to write it with
"clean up the most recent allocation".

Patch fixes two isues:
1. If pdev->dcbaa = dma_alloc_coherent() fails then that leads to a NULL
   dereference inside the cdnsp_free_priv_device() function.
2. if cdnsp_alloc_priv_device() fails that leads to a double free because
   we free pdev->out_ctx.bytes in several places.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Pawel Laszczak <pawell@cadence.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Tested-by: Pawel Laszczak <pawell@cadence.com>
---
 drivers/usb/cdns3/cdnsp-mem.c | 36 +++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/cdns3/cdnsp-mem.c b/drivers/usb/cdns3/cdnsp-mem.c
index 980047b7e416..4c7d77fb097e 100644
--- a/drivers/usb/cdns3/cdnsp-mem.c
+++ b/drivers/usb/cdns3/cdnsp-mem.c
@@ -1228,7 +1228,7 @@ int cdnsp_mem_init(struct cdnsp_device *pdev)
 	pdev->dcbaa = dma_alloc_coherent(dev, sizeof(*pdev->dcbaa),
 					 &dma, GFP_KERNEL);
 	if (!pdev->dcbaa)
-		goto mem_init_fail;
+		return -ENOMEM;
 
 	memset(pdev->dcbaa, 0, sizeof(*pdev->dcbaa));
 	pdev->dcbaa->dma = dma;
@@ -1246,17 +1246,19 @@ int cdnsp_mem_init(struct cdnsp_device *pdev)
 	pdev->segment_pool = dma_pool_create("CDNSP ring segments", dev,
 					     TRB_SEGMENT_SIZE, TRB_SEGMENT_SIZE,
 					     page_size);
+	if (!pdev->segment_pool)
+		goto release_dcbaa;
 
 	pdev->device_pool = dma_pool_create("CDNSP input/output contexts", dev,
 					    CDNSP_CTX_SIZE, 64, page_size);
+	if (!pdev->device_pool)
+		goto destroy_segment_pool;
 
-	if (!pdev->segment_pool || !pdev->device_pool)
-		goto mem_init_fail;
 
 	/* Set up the command ring to have one segments for now. */
 	pdev->cmd_ring = cdnsp_ring_alloc(pdev, 1, TYPE_COMMAND, 0, GFP_KERNEL);
 	if (!pdev->cmd_ring)
-		goto mem_init_fail;
+		goto destroy_device_pool;
 
 	/* Set the address in the Command Ring Control register */
 	val_64 = cdnsp_read_64(&pdev->op_regs->cmd_ring);
@@ -1279,11 +1281,11 @@ int cdnsp_mem_init(struct cdnsp_device *pdev)
 	pdev->event_ring = cdnsp_ring_alloc(pdev, ERST_NUM_SEGS, TYPE_EVENT,
 					    0, GFP_KERNEL);
 	if (!pdev->event_ring)
-		goto mem_init_fail;
+		goto free_cmd_ring;
 
 	ret = cdnsp_alloc_erst(pdev, pdev->event_ring, &pdev->erst);
 	if (ret)
-		goto mem_init_fail;
+		goto free_event_ring;
 
 	/* Set ERST count with the number of entries in the segment table. */
 	val = readl(&pdev->ir_set->erst_size);
@@ -1302,22 +1304,32 @@ int cdnsp_mem_init(struct cdnsp_device *pdev)
 
 	ret = cdnsp_setup_port_arrays(pdev);
 	if (ret)
-		goto mem_init_fail;
+		goto free_erst;
 
 	ret = cdnsp_alloc_priv_device(pdev);
 	if (ret) {
 		dev_err(pdev->dev,
 			"Could not allocate cdnsp_device data structures\n");
-		goto mem_init_fail;
+		goto free_erst;
 	}
 
 	return 0;
 
-mem_init_fail:
-	dev_err(pdev->dev, "Couldn't initialize memory\n");
-	cdnsp_halt(pdev);
+free_erst:
+	cdnsp_free_erst(pdev, &pdev->erst);
+free_event_ring:
+	cdnsp_ring_free(pdev, pdev->event_ring);
+free_cmd_ring:
+	cdnsp_ring_free(pdev, pdev->cmd_ring);
+destroy_device_pool:
+	dma_pool_destroy(pdev->device_pool);
+destroy_segment_pool:
+	dma_pool_destroy(pdev->segment_pool);
+release_dcbaa:
+	dma_free_coherent(dev, sizeof(*pdev->dcbaa), pdev->dcbaa,
+			  pdev->dcbaa->dma);
+
 	cdnsp_reset(pdev);
-	cdnsp_mem_cleanup(pdev);
 
 	return ret;
 }
-- 
2.17.1

Re: [PATCH] usb: cdnsp: fix error handling in cdnsp_mem_init()

[Dan Carpenter]

On Fri, Dec 11, 2020 at 10:50:53AM +0100, Pawel Laszczak wrote:
> This function uses "One Function Cleans up Everything" style and that's
> basically impossible to do correctly. It's cleaner to write it with
> "clean up the most recent allocation".
> 
> Patch fixes two isues:
> 1. If pdev->dcbaa = dma_alloc_coherent() fails then that leads to a NULL
>    dereference inside the cdnsp_free_priv_device() function.
> 2. if cdnsp_alloc_priv_device() fails that leads to a double free because
>    we free pdev->out_ctx.bytes in several places.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Tested-by: Pawel Laszczak <pawell@cadence.com>

Thanks so much!

regards,
dan carpenter

Re: [PATCH] usb: cdnsp: fix error handling in cdnsp_mem_init()

[Greg KH]

On Fri, Dec 11, 2020 at 10:50:53AM +0100, Pawel Laszczak wrote:
> This function uses "One Function Cleans up Everything" style and that's
> basically impossible to do correctly. It's cleaner to write it with
> "clean up the most recent allocation".
> 
> Patch fixes two isues:
> 1. If pdev->dcbaa = dma_alloc_coherent() fails then that leads to a NULL
>    dereference inside the cdnsp_free_priv_device() function.
> 2. if cdnsp_alloc_priv_device() fails that leads to a double free because
>    we free pdev->out_ctx.bytes in several places.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Tested-by: Pawel Laszczak <pawell@cadence.com>
> ---
>  drivers/usb/cdns3/cdnsp-mem.c | 36 +++++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 12 deletions(-)

This file isn't in 5.11-rc1 :(

RE: [PATCH] usb: cdnsp: fix error handling in cdnsp_mem_init()

[Pawel Laszczak]

>On Fri, Dec 11, 2020 at 10:50:53AM +0100, Pawel Laszczak wrote:
>> This function uses "One Function Cleans up Everything" style and that's
>> basically impossible to do correctly. It's cleaner to write it with
>> "clean up the most recent allocation".
>>
>> Patch fixes two isues:
>> 1. If pdev->dcbaa = dma_alloc_coherent() fails then that leads to a NULL
>>    dereference inside the cdnsp_free_priv_device() function.
>> 2. if cdnsp_alloc_priv_device() fails that leads to a double free because
>>    we free pdev->out_ctx.bytes in several places.
>>
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>> Tested-by: Pawel Laszczak <pawell@cadence.com>
>> ---
>>  drivers/usb/cdns3/cdnsp-mem.c | 36 +++++++++++++++++++++++------------
>>  1 file changed, 24 insertions(+), 12 deletions(-)
>
>This file isn't in 5.11-rc1 :(

Hi Greg,

Sorry for the long delay. I had holiday.

All CDNS3 and CDNSP patches should be added to Peter Chan tree,
so I based on his tree.

Regards,
Pawel