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

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 <[email protected]>

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/[email protected]?part=1

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