dmaengine: ppc4xx: check dma_map_page() errors in probe
PATCH 1/1 dmaengine: ppc4xx: check dma_map_page() errors in probe
In ppc440spe_adma_probe() the helper pages are mapped with dma_map_page() but the returned DMA address is never validated with dma_mapping_error(). On 440SPe the mapping goes through the SWIOTLB/direct map, which can fail under memory pressure or with an IOMMU, returning DMA_MAPPING_ERROR. The bogus address would then be programmed into the CDBs used by the async validation and mult/sum_product operations, causing the engine to DMA to or from arbitrary memory and corrupt data.
Fail the probe when either mapping fails, freeing the pages and unmapping the first page if the second mapping fails.
--- drivers/dma/ppc4xx/adma.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
drivers/dma/ppc4xx/adma.c +14 -0
@@ -4156,8 +4156,22 @@ static int ppc440spe_adma_probe(struct platform_device *ofdev) } chan->pdest =dma_map_page (&ofdev->dev, chan->pdest_page,0 ,PAGE_SIZE ,DMA_BIDIRECTIONAL ); +if (dma_mapping_error (&ofdev->dev, chan->pdest)) { +__free_page (chan->pdest_page); +__free_page (chan->qdest_page); + ret = -ENOMEM ; + goto out; + } chan->qdest =dma_map_page (&ofdev->dev, chan->qdest_page,0 ,PAGE_SIZE ,DMA_BIDIRECTIONAL ); +if (dma_mapping_error (&ofdev->dev, chan->qdest)) { +dma_unmap_page (&ofdev->dev, chan->pdest, +PAGE_SIZE ,DMA_BIDIRECTIONAL ); +__free_page (chan->pdest_page); +__free_page (chan->qdest_page); + ret = -ENOMEM ; + goto out; + } } ref =kmalloc_obj (*ref);
Signature · 1 line
2.55.0
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] The error paths for dma_map_page() failures use `goto out;`, bypassing all resource cleanup and…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The error paths for dma_map_page() failures use goto out;, bypassing all resource cleanup and causing a massive memory and resource leak. --
commit e542c1430356273e51891dbcad4333a7ba32009f
Author: Rosen Penev <[email protected]>
dmaengine: ppc4xx: check dma_map_page() errors in probe
This commit adds validation for the DMA addresses returned by dma_map_page() in ppc440spe_adma_probe(). It fails the probe and frees the pages when a mapping error occurs to avoid corrupting data.
Quoted 4 lines
drivers/dma/ppc4xx/adma.c +0
-0
[ ... ]
drivers/dma/ppc4xx/adma.c static int ppc440spe_adma_probe(struct platform_device *ofdev) · line 4163
Quoted 9 lines
drivers/dma/ppc4xx/adma.c +5
-0@@ -4156,8 +4156,22 @@ static int ppc440spe_adma_probe(struct platform_device *ofdev) } chan->pdest =dma_map_page (&ofdev->dev, chan->pdest_page,0 ,PAGE_SIZE ,DMA_BIDIRECTIONAL ); +if (dma_mapping_error (&ofdev->dev, chan->pdest)) { +__free_page (chan->pdest_page); +__free_page (chan->qdest_page); + ret = -ENOMEM ; + goto out;
[Severity: High]
Does jumping directly to out bypass the necessary cleanup for chan, adev, the dma_desc_pool, and the ioremap regions?
It looks like earlier errors in ppc440spe_adma_probe() jump to specific error labels (such as err_page_alloc) to unwind and free these resources. If the pdest mapping fails, jumping directly to out instead of the proper error labels appears to leak the channel structure and device resources.
drivers/dma/ppc4xx/adma.c static int ppc440spe_adma_probe(struct platform_device *ofdev) · line 4173
Quoted 10 lines
drivers/dma/ppc4xx/adma.c +8
-0+ } chan->qdest =dma_map_page (&ofdev->dev, chan->qdest_page,0 ,PAGE_SIZE ,DMA_BIDIRECTIONAL ); +if (dma_mapping_error (&ofdev->dev, chan->qdest)) { +dma_unmap_page (&ofdev->dev, chan->pdest, +PAGE_SIZE ,DMA_BIDIRECTIONAL ); +__free_page (chan->pdest_page); +__free_page (chan->qdest_page); + ret = -ENOMEM ; + goto out;
[Severity: High]
Similarly, does this goto also leak the chan structure, adev, dma_desc_pool, and ioremap regions when the qdest mapping fails?
Jumping to err_page_alloc might correctly unwind the allocations made earlier in the probe function after the pages are freed.
Quoted 4 lines
drivers/dma/ppc4xx/adma.c +1
-0+ } } ref =kmalloc_obj (*ref);
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Quoted 24 lines · Rosen Penev
In
ppc440spe_adma_probe()the helper pages are mapped withdma_map_page()but the returned DMA address is never validated withdma_mapping_error(). On 440SPe the mapping goes through the SWIOTLB/direct map, which can fail under memory pressure or with an IOMMU, returningDMA_MAPPING_ERROR. The bogus address would then be programmed into the CDBs used by the async validation and mult/sum_product operations, causing the engine to DMA to or from arbitrary memory and corrupt data.Fail the probe when either mapping fails, freeing the pages and unmapping the first page if the second mapping fails.
Fixes 12458ea06efd7 ("ppc440spe-adma: adds updated ppc440spe adma driver")Assisted-by opencode:big-pickleSigned-off-by Rosen Penev <[email protected]>--- drivers/dma/ppc4xx/adma.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)drivers/dma/ppc4xx/adma.c +0
-0
drivers/dma/ppc4xx/adma.c static int ppc440spe_adma_probe(struct platform_device *ofdev) · line 4161
Quoted 6 lines
drivers/dma/ppc4xx/adma.c +3
-0@@ -4156,8 +4156,22 @@ static int ppc440spe_adma_probe(struct platform_device *ofdev) } chan->pdest =dma_map_page (&ofdev->dev, chan->pdest_page,0 ,PAGE_SIZE ,DMA_BIDIRECTIONAL ); +if (dma_mapping_error (&ofdev->dev, chan->pdest)) { +__free_page (chan->pdest_page); +__free_page (chan->qdest_page);
put these to lable out
Frank
Quoted 19 lines
drivers/dma/ppc4xx/adma.c +11
-1+ ret = -ENOMEM ; + goto out; + } chan->qdest =dma_map_page (&ofdev->dev, chan->qdest_page,0 ,PAGE_SIZE ,DMA_BIDIRECTIONAL ); +if (dma_mapping_error (&ofdev->dev, chan->qdest)) { +dma_unmap_page (&ofdev->dev, chan->pdest, +PAGE_SIZE ,DMA_BIDIRECTIONAL ); +__free_page (chan->pdest_page); +__free_page (chan->qdest_page); + ret = -ENOMEM ; + goto out; + } } ref =kmalloc_obj (*ref); --2.55.0
Quoted 20 lines · Frank Li
Quoted 19 lines · Rosen Penev
In
ppc440spe_adma_probe()the helper pages are mapped withdma_map_page()but the returned DMA address is never validated withdma_mapping_error(). On 440SPe the mapping goes through the SWIOTLB/direct map, which can fail under memory pressure or with an IOMMU, returningDMA_MAPPING_ERROR. The bogus address would then be programmed into the CDBs used by the async validation and mult/sum_product operations, causing the engine to DMA to or from arbitrary memory and corrupt data.Fail the probe when either mapping fails, freeing the pages and unmapping the first page if the second mapping fails.
Fixes 12458ea06efd7 ("ppc440spe-adma: adds updated ppc440spe adma driver")Assisted-by opencode:big-pickleSigned-off-by Rosen Penev <[email protected]>--- drivers/dma/ppc4xx/adma.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
Quoted 13 lines
drivers/dma/ppc4xx/adma.c static int ppc440spe_adma_probe(struct platform_device *ofdev) · line 4161
Quoted 11 lines
drivers/dma/ppc4xx/adma.c +3
-0@@ -4156,8 +4156,22 @@ static int ppc440spe_adma_probe(struct platform_device *ofdev) } chan->pdest =dma_map_page (&ofdev->dev, chan->pdest_page,0 ,PAGE_SIZE ,DMA_BIDIRECTIONAL ); +if (dma_mapping_error (&ofdev->dev, chan->pdest)) { +__free_page (chan->pdest_page); +__free_page (chan->qdest_page);put these to lable out
Problem there is this is in an if statement. It's not as clean to handle here directly.
Quoted 22 lines
Frank
Quoted 19 lines
drivers/dma/ppc4xx/adma.c +11
-1+ ret = -ENOMEM ; + goto out; + } chan->qdest =dma_map_page (&ofdev->dev, chan->qdest_page,0 ,PAGE_SIZE ,DMA_BIDIRECTIONAL ); +if (dma_mapping_error (&ofdev->dev, chan->qdest)) { +dma_unmap_page (&ofdev->dev, chan->pdest, +PAGE_SIZE ,DMA_BIDIRECTIONAL ); +__free_page (chan->pdest_page); +__free_page (chan->qdest_page); + ret = -ENOMEM ; + goto out; + } } ref =kmalloc_obj (*ref); --2.55.0