[PATCH] crypto: mxs-dcp - fix source scatterlist length access

[PATCH] crypto: mxs-dcp - fix source scatterlist length access

[Thorsten Blum]

mxs_dcp_aes_block_crypt() uses sg_dma_len() without mapping the source
scatterlist with dma_map_sg() first. Therefore, sg_dma_len() is invalid
and could return zero or a stale DMA length, causing encryption and
decryption to process the wrong number of bytes when
CONFIG_NEED_SG_DMA_LENGTH=y.

Use the original scatterlist length instead.

Fixes: 15b59e7c3733 ("crypto: mxs - Add Freescale MXS DCP driver")
Cc: stable@vger.kernel.org
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
 drivers/crypto/mxs-dcp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/mxs-dcp.c b/drivers/crypto/mxs-dcp.c
index 133ebc998236..595b2fd84667 100644
--- a/drivers/crypto/mxs-dcp.c
+++ b/drivers/crypto/mxs-dcp.c
@@ -353,7 +353,7 @@ static int mxs_dcp_aes_block_crypt(struct crypto_async_request *arq)
 
 	for_each_sg(req->src, src, sg_nents(req->src), i) {
 		src_buf = sg_virt(src);
-		len = sg_dma_len(src);
+		len = src->length;
 		tlen += len;
 		limit_hit = tlen > req->cryptlen;
 

Re: [PATCH] crypto: mxs-dcp - fix source scatterlist length access

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Processing a 0-length skcipher request in CBC mode causes an integer underflow and an out-of-bounds memory read when updating the initialization vector (IV), potentially disclosing kernel heap/SLAB memory.
- [High] The driver uses `sg_virt(src)` to access source scatterlist buffers directly, which returns a bad pointer (usually NULL) for pages in high memory (HIGHMEM), leading to a kernel crash during `memcpy()`.
- [High] The DMA descriptor mapping `desc_phys` is leaked on error exit paths in `mxs_dcp_start_dma()`.
- [High] When the platform device is unbound, active TFMs are not properly drained, leading to a Null Pointer Dereference on global_sdcp. Additionally, pending requests in the async queues are abandoned and leaked.
--

commit 654ea836acbafd7d2841dcce57671e9d5ef8944d
Author: Thorsten Blum <thorsten.blum@linux.dev>

crypto: mxs-dcp - fix source scatterlist length access

mxs_dcp_aes_block_crypt() uses sg_dma_len() without mapping the source
scatterlist with dma_map_sg() first. Therefore, sg_dma_len() is invalid
and could return zero or a stale DMA length, causing encryption and
decryption to process the wrong number of bytes when
CONFIG_NEED_SG_DMA_LENGTH=y. Use the original scatterlist length instead.

> diff --git a/drivers/crypto/mxs-dcp.c b/drivers/crypto/mxs-dcp.c
> index 133ebc9982362..595b2fd84667e 100644
> --- a/drivers/crypto/mxs-dcp.c
> +++ b/drivers/crypto/mxs-dcp.c
> @@ -353,7 +353,7 @@ static int mxs_dcp_aes_block_crypt(struct crypto_async_request *arq)
>  
>  	for_each_sg(req->src, src, sg_nents(req->src), i) {
>  		src_buf = sg_virt(src);
> -		len = sg_dma_len(src);
> +		len = src->length;

[Severity: High]
This is a pre-existing issue, but does using sg_virt() here safely handle high
memory pages in drivers/crypto/mxs-dcp.c:mxs_dcp_aes_block_crypt()?

If the crypto request originates from userspace (e.g., via AF_ALG) and passes
pages allocated in high memory (HIGHMEM), sg_virt() could return NULL because
the page isn't mapped into the kernel's linear address space.

Would this lead to a NULL pointer dereference when memcpy() is called later on
src_buf?

>  		tlen += len;
>  		limit_hit = tlen > req->cryptlen;

[ ... ]

[Severity: Critical]
This is a pre-existing issue, but I noticed a potential underflow when
computing the IV out-of-bounds offset in
drivers/crypto/mxs-dcp.c:mxs_dcp_aes_block_crypt():

	/* Copy the IV for CBC for chaining */
	if (!rctx->ecb) {
		if (rctx->enc)
			memcpy(req->iv, out_buf+(last_out_len-AES_BLOCK_SIZE),
				AES_BLOCK_SIZE);

If a user submits a 0-length encryption request, the for_each_sg loop may exit
without updating last_out_len, leaving it at 0.

Since last_out_len is an unsigned 32-bit integer, what happens when subtracting
AES_BLOCK_SIZE (16)? Will it underflow to 0xfffffff0?

If so, this could read 16 bytes before the start of the in_buf or out_buf,
potentially disclosing sensitive memory into the IV.

[Severity: High]
This is a pre-existing issue, but could the DMA mapping desc_phys be leaked
on error paths in drivers/crypto/mxs-dcp.c:mxs_dcp_start_dma()?

	ret = wait_for_completion_timeout(&sdcp->completion[chan],
					  msecs_to_jiffies(1000));
	if (!ret) {
		dev_err(sdcp->dev, "Channel %i timeout (DCP_STAT=0x%08x)\n",
			chan, readl(sdcp->base + MXS_DCP_STAT));
		return -ETIMEDOUT;
	}

If the DMA operation times out or the channel status indicates a hardware error,
the function returns directly.

Does this bypass the dma_unmap_single() call at the end of the function,
leaving IOMMU mappings or DMA bounce buffer space allocated?

[Severity: High]
This is a pre-existing issue, but is the driver unbind sequence in
drivers/crypto/mxs-dcp.c:mxs_dcp_remove() properly tearing down active TFMs?

	kthread_stop(sdcp->thread[DCP_CHAN_HASH_SHA]);
	kthread_stop(sdcp->thread[DCP_CHAN_CRYPTO]);

	platform_set_drvdata(pdev, NULL);

	global_sdcp = NULL;

If active TFMs are not destroyed (for example, if held open by AF_ALG sockets),
could a new request submitted after unbind cause mxs_dcp_aes_enqueue() to
dereference the now-NULL global_sdcp pointer?

Also, calling kthread_stop() causes the crypto worker threads to exit. Are
requests currently queued in sdcp->queue abandoned, leaving callers waiting
for completions indefinitely?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260621192615.277957-2-thorsten.blum@linux.dev?part=1

Re: [PATCH] crypto: mxs-dcp - fix source scatterlist length access

[Frank Li]

>
> mxs_dcp_aes_block_crypt() uses sg_dma_len() without mapping the source
> scatterlist with dma_map_sg() first. Therefore, sg_dma_len() is invalid
> and could return zero or a stale DMA length, causing encryption and
> decryption to process the wrong number of bytes when
> CONFIG_NEED_SG_DMA_LENGTH=y.
>
> Use the original scatterlist length instead.

It'd beter add some description about copy sg to a bounce buffer to do
descrytpion, needn't map sg firstly and direct use the original
scatterlist length.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
>
> Fixes: 15b59e7c3733 ("crypto: mxs - Add Freescale MXS DCP driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
>  drivers/crypto/mxs-dcp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/crypto/mxs-dcp.c b/drivers/crypto/mxs-dcp.c
> index 133ebc998236..595b2fd84667 100644
> --- a/drivers/crypto/mxs-dcp.c
> +++ b/drivers/crypto/mxs-dcp.c
> @@ -353,7 +353,7 @@ static int mxs_dcp_aes_block_crypt(struct crypto_async_request *arq)
>
>         for_each_sg(req->src, src, sg_nents(req->src), i) {
>                 src_buf = sg_virt(src);
> -               len = sg_dma_len(src);
> +               len = src->length;
>                 tlen += len;
>                 limit_hit = tlen > req->cryptlen;
>
>