From: Shravya Kumbham <redacted>
In zynqmp_dma_alloc/free_chan_resources functions there is a
potential overflow in the below expressions.
dma_alloc_coherent(chan->dev, (2 * chan->desc_size *
ZYNQMP_DMA_NUM_DESCS),
&chan->desc_pool_p, GFP_KERNEL);
dma_free_coherent(chan->dev,(2 * ZYNQMP_DMA_DESC_SIZE(chan) *
ZYNQMP_DMA_NUM_DESCS),
chan->desc_pool_v, chan->desc_pool_p);
The arguments desc_size and ZYNQMP_DMA_NUM_DESCS are 32 bit. Though
this overflow condition is not observed but it is a potential problem
in the case of 32-bit multiplication. Hence fix it by using typecast.
Addresses-Coverity: Event overflow_before_widen.
Signed-off-by: Shravya Kumbham <redacted>
Reviewed-by: Radhey Shyam Pandey <redacted>
---
drivers/dma/xilinx/zynqmp_dma.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
From: Shravya Kumbham <redacted>
In zynqmp_dma_alloc_chan_resources function there is a potential
overflow in the below expression.
desc->src_p = chan->desc_pool_p + (i * ZYNQMP_DMA_DESC_SIZE(chan*2);
The macro ZYNQMP_DMA_DESC_SIZE and variable i are 32-bit. Though
this overflow condition is not observed but it is a potential problem
in the case of 32-bit multiplication. Hence fix it by using typecast.
Addresses-Coverity: Event overflow_before_widen.
Signed-off-by: Shravya Kumbham <redacted>
Reviewed-by: Radhey Shyam Pandey <redacted>
---
drivers/dma/xilinx/zynqmp_dma.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
@@ -1080,7 +1080,11 @@ static int zynqmp_dma_probe(struct platform_device *pdev)pm_runtime_set_autosuspend_delay(zdev->dev,ZDMA_PM_TIMEOUT);pm_runtime_use_autosuspend(zdev->dev);pm_runtime_enable(zdev->dev);-pm_runtime_get_sync(zdev->dev);+ret=pm_runtime_get_sync(zdev->dev);+if(ret<0){+dev_err(zdev->dev,"pm_runtime_get_sync() failed\n");+pm_runtime_disable(zdev->dev);+}if(!pm_runtime_enabled(zdev->dev)){ret=zynqmp_dma_runtime_resume(zdev->dev);if(ret)
@@ -1096,7 +1100,11 @@ static int zynqmp_dma_probe(struct platform_device *pdev)p->dst_addr_widths=BIT(zdev->chan->bus_width/8);p->src_addr_widths=BIT(zdev->chan->bus_width/8);-dma_async_device_register(&zdev->common);+ret=dma_async_device_register(&zdev->common);+if(ret){+dev_err(zdev->dev,"failed to register the dma device\n");+gotofree_chan_resources;+}ret=of_dma_controller_register(pdev->dev.of_node,of_zynqmp_dma_xlate,zdev);
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Michael Tretter <m.tretter@pengutronix.de> Date: 2021-09-23 14:12:16
Hi Harini,
On Tue, 14 Sep 2021 13:58:14 +0530, Harini Katakam wrote:
From: Shravya Kumbham <redacted>
In zynqmp_dma_alloc/free_chan_resources functions there is a
potential overflow in the below expressions.
dma_alloc_coherent(chan->dev, (2 * chan->desc_size *
ZYNQMP_DMA_NUM_DESCS),
&chan->desc_pool_p, GFP_KERNEL);
dma_free_coherent(chan->dev,(2 * ZYNQMP_DMA_DESC_SIZE(chan) *
ZYNQMP_DMA_NUM_DESCS),
chan->desc_pool_v, chan->desc_pool_p);
The arguments desc_size and ZYNQMP_DMA_NUM_DESCS are 32 bit. Though
this overflow condition is not observed but it is a potential problem
in the case of 32-bit multiplication. Hence fix it by using typecast.
Addresses-Coverity: Event overflow_before_widen.
Signed-off-by: Shravya Kumbham <redacted>
Reviewed-by: Radhey Shyam Pandey <redacted>
Thanks for the patch.
Your SoB is missing in this and the other patches of this series.
@@ -490,7 +490,8 @@ static int zynqmp_dma_alloc_chan_resources(struct dma_chan *dchan)}chan->desc_pool_v=dma_alloc_coherent(chan->dev,-(2*chan->desc_size*ZYNQMP_DMA_NUM_DESCS),+((size_t)(2*chan->desc_size)*+ZYNQMP_DMA_NUM_DESCS),
Wouldn't it be easier to change the type of chan->desc_size to size_t? Maybe
we could also just calculate the size of the descriptor pool during probe() to
make the code more readable?
I also noticed that there is the ZYNQMP_DMA_DESC_SIZE() macro, which is
inconsistently used in the driver. Maybe you could cleanup this as well as you
are at it?
quoted hunk
&chan->desc_pool_p, GFP_KERNEL);
if (!chan->desc_pool_v)
return -ENOMEM;
Thanks for the patch.
I looked at a few dmaengine drivers, at it looks like all of them have this
issue. Maybe we should change the signature of the dmaengine_prep_dma_memcpy()
engine to accept "enum dma_ctrl_flags flags" instead of "unsigned long flags"?
Michael
@@ -1080,7 +1080,11 @@ static int zynqmp_dma_probe(struct platform_device *pdev)pm_runtime_set_autosuspend_delay(zdev->dev,ZDMA_PM_TIMEOUT);pm_runtime_use_autosuspend(zdev->dev);pm_runtime_enable(zdev->dev);-pm_runtime_get_sync(zdev->dev);+ret=pm_runtime_get_sync(zdev->dev);+if(ret<0){+dev_err(zdev->dev,"pm_runtime_get_sync() failed\n");+pm_runtime_disable(zdev->dev);+}
Thanks for the patch.
You need to call pm_runtime_put() on the error path. Or you could use
pm_runtime_resume_and_get(), which does this automatically.
I am wondering, if it wouldn't be cleaner to make this dependent on
pm_runtime_enabled() and avoiding pm_runtime_disable() on the error path
altogether.
Michael
quoted hunk
if (!pm_runtime_enabled(zdev->dev)) {
ret = zynqmp_dma_runtime_resume(zdev->dev);
if (ret)
@@ -1096,7 +1100,11 @@ static int zynqmp_dma_probe(struct platform_device *pdev) p->dst_addr_widths = BIT(zdev->chan->bus_width / 8); p->src_addr_widths = BIT(zdev->chan->bus_width / 8);- dma_async_device_register(&zdev->common);+ ret = dma_async_device_register(&zdev->common);+ if (ret) {+ dev_err(zdev->dev, "failed to register the dma device\n");+ goto free_chan_resources;+ } ret = of_dma_controller_register(pdev->dev.of_node, of_zynqmp_dma_xlate, zdev);
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Shravya Kumbham <redacted>
In zynqmp_dma_alloc/free_chan_resources functions there is a
potential overflow in the below expressions.
dma_alloc_coherent(chan->dev, (2 * chan->desc_size *
ZYNQMP_DMA_NUM_DESCS),
&chan->desc_pool_p, GFP_KERNEL);
dma_free_coherent(chan->dev,(2 * ZYNQMP_DMA_DESC_SIZE(chan) *
ZYNQMP_DMA_NUM_DESCS),
chan->desc_pool_v, chan->desc_pool_p);
The arguments desc_size and ZYNQMP_DMA_NUM_DESCS are 32 bit. Though
this overflow condition is not observed but it is a potential problem
in the case of 32-bit multiplication. Hence fix it by using typecast.
Addresses-Coverity: Event overflow_before_widen.
Signed-off-by: Shravya Kumbham <redacted>
Patch was sent by Harini Katakam [off-list ref] and SOB not
available for person sending this patch, sorry cant accept it with
s-o-b...
From: Shravya Kumbham <redacted>
Add condition to check the return value of dma_async_device_register
and pm_runtime_get_sync functions.
Addresses-Coverity: Event check_return.
Signed-off-by: Shravya Kumbham <redacted>