[PATCH v2] crypto: amlogic: Fix DMA memory leak in cipher error path

[PATCH v2] crypto: amlogic: Fix DMA memory leak in cipher error path

[Mohamad Raizudeen]

A DMA memory leak occurs in meson_cipher() on the mapping error paths.
The driver jumps to the end of the function without unmapping the
previously mapped source scatterlist and key/IV buffer when the
destination apping fails.

Additionally, a memory leak occurs when a scatterlist mapping succeeds
but the returned count exceeds the driver's MAXDESC limit. In this case,
the driver rejects the mapping without unmapping it. The BIDIRECTIONAL
mapping branch also lacks the required 'MAXDESC -3' upper bound check.

Fix this by introducing proper error labels, error_src and error_keyiv
and unmap resources immediately inside the calidation checks to ensure
all successfully mapped resources are cleaned up before returning the
error.

Fixes: 48fe583fe541 ("crypto: amlogic - Add crypto accelerator for amlogic GXL")
Signed-off-by: Mohamad Raizudeen <raizudeen.kerneldev@gmail.com>
---
Changes in v2:
- Add missing '> MAXDESC - 3' upper bound check for the BIDIRECTIONAL
  scatterlist mapping branch.
- Unmap scatterlists immediately inside the 'if' block when the count
  exceeds MAXDESC - 3, because dma_map_sg() actually succeeds in this
  case and the mapping must be undone.

 drivers/crypto/amlogic/amlogic-gxl-cipher.c | 25 ++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/crypto/amlogic/amlogic-gxl-cipher.c b/drivers/crypto/amlogic/amlogic-gxl-cipher.c
index 29048da6f50a..39b44ec89997 100644
--- a/drivers/crypto/amlogic/amlogic-gxl-cipher.c
+++ b/drivers/crypto/amlogic/amlogic-gxl-cipher.c
@@ -177,10 +177,13 @@ static int meson_cipher(struct skcipher_request *areq)
 	if (areq->src == areq->dst) {
 		nr_sgs = dma_map_sg(mc->dev, areq->src, sg_nents(areq->src),
 				    DMA_BIDIRECTIONAL);
-		if (!nr_sgs) {
-			dev_err(mc->dev, "Invalid SG count %d\n", nr_sgs);
+		if (!nr_sgs || nr_sgs > MAXDESC - 3) {
+			dev_err(mc->dev, "Invalid BIDIR SG count %d\n", nr_sgs);
 			err = -EINVAL;
-			goto theend;
+			
+			if (nr_sgs)
+				dma_unmap_sg(mc->dev, areq->src, sg_nents(areq->src), DMA_BIDIRECTIONAL);
+			goto error_keyiv;
 		}
 		nr_sgd = nr_sgs;
 	} else {
@@ -189,14 +192,20 @@ static int meson_cipher(struct skcipher_request *areq)
 		if (!nr_sgs || nr_sgs > MAXDESC - 3) {
 			dev_err(mc->dev, "Invalid SG count %d\n", nr_sgs);
 			err = -EINVAL;
-			goto theend;
+
+			if (nr_sgs)
+				dma_unmap_sg(mc->dev, areq->src, sg_nents(areq->src), DMA_TO_DEVICE);
+			goto error_keyiv;
 		}
 		nr_sgd = dma_map_sg(mc->dev, areq->dst, sg_nents(areq->dst),
 				    DMA_FROM_DEVICE);
 		if (!nr_sgd || nr_sgd > MAXDESC - 3) {
 			dev_err(mc->dev, "Invalid SG count %d\n", nr_sgd);
 			err = -EINVAL;
-			goto theend;
+
+			if (nr_sgd)
+				dma_unmap_sg(mc->dev, areq->dst, sg_nents(areq->dst), DMA_FROM_DEVICE);
+			goto error_src;
 		}
 	}
 
@@ -251,6 +260,12 @@ static int meson_cipher(struct skcipher_request *areq)
 						 ivsize, 0);
 		}
 	}
+	goto theend;
+
+error_src:
+	dma_unmap_sg(mc->dev, areq->src, sg_nents(areq->src), DMA_TO_DEVICE);
+error_keyiv:
+	dma_unmap_single(mc->dev, phykeyiv, keyivlen, DMA_TO_DEVICE);
 theend:
 	kfree_sensitive(bkeyiv);
 	kfree_sensitive(backup_iv);
-- 
2.53.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v2] crypto: amlogic: Fix DMA memory leak in cipher error path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] The scatterlist iteration loop ignores IOMMU coalescing, leading to hardware descriptor array overflow and invalid DMA address configuration.
- [High] Synchronous lockstep traversal of source and destination scatterlists incorrectly assumes identical scatterlist topologies, risking NULL pointer dereferences and out-of-bounds DMA writes.
- [High] Uncanceled hardware DMA operations on software timeout or signal interrupt cause Use-After-Free and memory corruption.
--

--- Patch [1]: [PATCH v2] crypto: amlogic: Fix DMA memory leak in cipher error path ---
commit eeaa7271320427ce2470dd81388b45f51f9e8d3f
Author: Mohamad Raizudeen <raizudeen.kerneldev@gmail.com>

crypto: amlogic: Fix DMA memory leak in cipher error path
    
This commit fixes a DMA memory leak in meson_cipher() mapping error paths by
ensuring previously mapped scatterlists and key/IV buffers are properly
unmapped when subsequent destination mappings fail. It also adds a missing
upper bound check for the bidirectional scatterlist mapping branch.

> diff --git a/drivers/crypto/amlogic/amlogic-gxl-cipher.c b/drivers/crypto/amlogic/amlogic-gxl-cipher.c
> index 29048da6f50a7..39b44ec899970 100644
> --- a/drivers/crypto/amlogic/amlogic-gxl-cipher.c
> +++ b/drivers/crypto/amlogic/amlogic-gxl-cipher.c
> @@ -177,10 +177,13 @@ static int meson_cipher(struct skcipher_request *areq)
>  	if (areq->src == areq->dst) {
>  		nr_sgs = dma_map_sg(mc->dev, areq->src, sg_nents(areq->src),
>  				    DMA_BIDIRECTIONAL);
> -		if (!nr_sgs) {
> -			dev_err(mc->dev, "Invalid SG count %d\n", nr_sgs);
> +		if (!nr_sgs || nr_sgs > MAXDESC - 3) {
> +			dev_err(mc->dev, "Invalid BIDIR SG count %d\n", nr_sgs);
>  			err = -EINVAL;
> -			goto theend;
> +			
> +			if (nr_sgs)
> +				dma_unmap_sg(mc->dev, areq->src, sg_nents(areq->src), DMA_BIDIRECTIONAL);
> +			goto error_keyiv;
>  		}
>  		nr_sgd = nr_sgs;
>  	} else {
> @@ -189,14 +192,20 @@ static int meson_cipher(struct skcipher_request *areq)
>  		if (!nr_sgs || nr_sgs > MAXDESC - 3) {
>  			dev_err(mc->dev, "Invalid SG count %d\n", nr_sgs);
>  			err = -EINVAL;
> -			goto theend;
> +
> +			if (nr_sgs)
> +				dma_unmap_sg(mc->dev, areq->src, sg_nents(areq->src), DMA_TO_DEVICE);
> +			goto error_keyiv;
>  		}
>  		nr_sgd = dma_map_sg(mc->dev, areq->dst, sg_nents(areq->dst),
>  				    DMA_FROM_DEVICE);
>  		if (!nr_sgd || nr_sgd > MAXDESC - 3) {
>  			dev_err(mc->dev, "Invalid SG count %d\n", nr_sgd);
>  			err = -EINVAL;
> -			goto theend;
> +
> +			if (nr_sgd)
> +				dma_unmap_sg(mc->dev, areq->dst, sg_nents(areq->dst), DMA_FROM_DEVICE);
> +			goto error_src;
>  		}
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but while we're correctly bounds
checking the IOMMU mapped scatterlist count (nr_sgs and nr_sgd) against
MAXDESC - 3, the later descriptor setup loop iterates based on the original
unmapped scatterlist entries:

meson_cipher() {
	...
	while (src_sg) {
		desc = &mc->chanlist[flow].tl[tloffset];
		...
		src_sg = sg_next(src_sg);
		dst_sg = sg_next(dst_sg);
	}
	...
}

Could a highly fragmented scatterlist with an original entry count exceeding
MAXDESC - 3, which is then coalesced by the IOMMU down to a valid nr_sgs,
cause this loop to overflow the fixed-size mc->chanlist[flow].tl[] descriptor
array?

Would this also pass unmapped, trailing scatterlist addresses to the hardware?

[Severity: High]
This is a pre-existing issue in the same descriptor setup loop:

meson_cipher() {
	...
	while (src_sg) {
		...
		desc->t_src = cpu_to_le32(sg_dma_address(src_sg));
		desc->t_dst = cpu_to_le32(sg_dma_address(dst_sg));
		...
		src_sg = sg_next(src_sg);
		dst_sg = sg_next(dst_sg);
	}
	...
}

Can this cause a NULL pointer dereference if the destination scatterlist has
fewer segments than the source scatterlist?

If an asynchronous crypto request has a different topology for source and
destination (such as 4 fragmented source pages but 1 contiguous destination
page), would sg_dma_address(dst_sg) fault when dst_sg becomes NULL before
src_sg?

Would this also enforce the src_sg lengths onto the dst_sg DMA transactions,
causing out-of-bounds hardware DMA writes?

[Severity: High]
This is a pre-existing issue, but does the error path properly halt the
DMA engine when a timeout or signal interrupt occurs?

meson_cipher() {
	...
	wait_for_completion_interruptible_timeout(&mc->chanlist[flow].complete,
						  msecs_to_jiffies(500));
	if (mc->chanlist[flow].status == 0) {
		dev_err(mc->dev, "DMA timeout for flow %d\n", flow);
		err = -EINVAL;
	}

	dma_unmap_single(mc->dev, phykeyiv, keyivlen, DMA_TO_DEVICE);
	...
	kfree_sensitive(bkeyiv);
	...
}

If the hardware DMA operation takes longer than 500ms or is interrupted by a
signal (like -ERESTARTSYS), the driver sets an error code, unmaps the DMA
scatterlists, and frees the internal key buffers.

However, the hardware DMA engine doesn't appear to be stopped or reset here.

Can this cause a use-after-free of the heap-allocated keys and silent memory
corruption if the actively running hardware asynchronously continues reading
from and writing to the reallocated physical memory pages?

> @@ -251,6 +260,12 @@ static int meson_cipher(struct skcipher_request *areq)
>  						 ivsize, 0);
>  		}
>  	}
> +	goto theend;
> +
> +error_src:
> +	dma_unmap_sg(mc->dev, areq->src, sg_nents(areq->src), DMA_TO_DEVICE);
> +error_keyiv:
> +	dma_unmap_single(mc->dev, phykeyiv, keyivlen, DMA_TO_DEVICE);
>  theend:
>  	kfree_sensitive(bkeyiv);
>  	kfree_sensitive(backup_iv);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260822084509.26385-1-raizudeen.kerneldev@gmail.com?part=1

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic