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

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

Pre-existing issues:
- [Critical] Missing upper bound validation for bidirectional DMA mapping leads to buffer overflow on hardware descriptor array.
- [Critical] The scatterlist processing loop iterates using original unmapped SG entries and assumes identical segmentation between source and destination.
- [High] The patch fails to fully resolve the DMA memory leaks it claims to fix when scatterlist lengths exceed driver limits.
- [High] Hardware engine is not halted on wait completion timeout or signal interrupt, leading to DMA Use-After-Free.
--

--- Patch [1]: [PATCH] crypto: amlogic: Fix DMA memory leak in cipher error path ---
commit 347bea6e349b31b4848db23f6d3f056f03d3fd58
Author: Mohamad Raizudeen <[email protected]>

crypto: amlogic: Fix DMA memory leak in cipher error path

In meson_cipher(), if the mapping of the destination scatterlist fails,
the driver jumps to the end of the function. This skips the unmapping of
the previously mapped source scatterlist and the key/IV buffer, causing
a DMA memory leak.

> diff --git a/drivers/crypto/amlogic/amlogic-gxl-cipher.c b/drivers/crypto/amlogic/amlogic-gxl-cipher.c
> index 29048da6f50a7..fc8569fd83fd0 100644
> --- a/drivers/crypto/amlogic/amlogic-gxl-cipher.c
> +++ b/drivers/crypto/amlogic/amlogic-gxl-cipher.c
> @@ -178,9 +178,9 @@ static int meson_cipher(struct skcipher_request *areq)
>  		nr_sgs = dma_map_sg(mc->dev, areq->src, sg_nents(areq->src),
>  				    DMA_BIDIRECTIONAL);
>  		if (!nr_sgs) {

[Severity: Critical]
This is a pre-existing issue, but is there a missing upper bound check here?

If the scatterlist maps to a count greater than MAXDESC - 3, the subsequent
processing loop will unconditionally iterate and write hardware descriptors
out of bounds on the mc->chanlist[flow].tl array.

Should nr_sgs be validated against MAXDESC - 3 as it is in the DMA_TO_DEVICE
branch?

> -			dev_err(mc->dev, "Invalid SG count %d\n", nr_sgs);
> +			dev_err(mc->dev, "Invalid BIDIR SG count %d\n", nr_sgs);
>  			err = -EINVAL;
> -			goto theend;
> +			goto error_keyiv;
>  		}
>  		nr_sgd = nr_sgs;
>  	} else {
> @@ -189,14 +189,14 @@ 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;
> +			goto error_keyiv;

[Severity: High]
This isn't a bug introduced by this patch, but since the goal of this
commit is to fix DMA memory leaks, does jumping to error_keyiv here fail
to unmap the successfully mapped areq->src?

If nr_sgs > MAXDESC - 3, dma_map_sg() succeeded, but error_keyiv only
unmaps the phykeyiv resource, leaving areq->src mapped.

>  		}
>  		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;
> +			goto error_src;

[Severity: High]
Similarly, this is a pre-existing issue, but does jumping to error_src
fail to unmap the successfully mapped areq->dst?

If nr_sgd > MAXDESC - 3, dma_map_sg() succeeded for areq->dst. The
error_src label will unmap areq->src, but areq->dst remains mapped.

>  		}
>  	}

[ ... ]

Additionally, I noticed a few other pre-existing issues in this same function
that might be worth addressing:

[Severity: Critical]
This is a pre-existing issue, but does the scatterlist processing loop iterate
incorrectly if dma_map_sg() coalesces segments?

drivers/crypto/amlogic/amlogic-gxl-cipher.c:meson_cipher() {
    ...
	src_sg = areq->src;
	dst_sg = areq->dst;
	len = areq->cryptlen;
	while (src_sg) {
		desc = &mc->chanlist[flow].tl[tloffset];
		memset(desc, 0, sizeof(struct meson_desc));

		desc->t_src = cpu_to_le32(sg_dma_address(src_sg));
		desc->t_dst = cpu_to_le32(sg_dma_address(dst_sg));
    ...
}

The loop iterates over the original unmapped SG entries using while (src_sg)
instead of the mapped count (nr_sgs).

If dma_map_sg() coalesces segments, trailing iterations will use stale DMA
addresses.

Furthermore, if areq->dst is less fragmented and has fewer entries than
areq->src, sg_next(dst_sg) will prematurely return NULL, leading to a NULL
pointer dereference when accessing sg_dma_address(dst_sg).

[Severity: High]
This is a pre-existing issue, but can a timeout or signal cause a DMA
use-after-free?

drivers/crypto/amlogic/amlogic-gxl-cipher.c: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);
    ...
}

If wait_for_completion_interruptible_timeout() times out or is interrupted by
a signal, the driver sets an error code, unmaps the DMA buffers, and returns
control to the caller who frees the buffers.

However, the hardware engine is never halted, meaning it continues its
asynchronous DMA operations on the now-unmapped physical memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic