Re: [PATCH v2] crypto: mxs-dcp: handle zero-length skcipher requests

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Out-of-bounds read and 0-length hardware submission due to improper scatterlist trailing data handling.
- [High] Potential deadlock due to using spin_lock() instead of spin_lock_bh() in queue management.
- [High] Invalid memory access and API violation using sg_virt() and sg_dma_len() on unmapped scatterlists.
- [High] DMA channel is not halted or reset on timeout, risking memory corruption.
--

commit e69f94141c2ba5f872a9844bc79817c711aedefd
Author: Linmao Li <lilinmao@kylinos.cn>

crypto: mxs-dcp: handle zero-length skcipher requests

The generic ECB and CBC implementations return success for zero-length
requests, but MXS-DCP queues them anyway. When such a request reaches the
worker, the CBC completion path subtracts AES_BLOCK_SIZE from last_out_len,
causing the offset to underflow. This commit returns success before
enqueueing zero-length requests to avoid the invalid source access.

> diff --git a/drivers/crypto/mxs-dcp.c b/drivers/crypto/mxs-dcp.c
> index 133ebc9982362..60794b4d49fac 100644
> --- a/drivers/crypto/mxs-dcp.c
> +++ b/drivers/crypto/mxs-dcp.c
> @@ -473,6 +473,9 @@ static int mxs_dcp_aes_enqueue(struct skcipher_request *req, int enc, int ecb)
>  	struct dcp_aes_req_ctx *rctx = skcipher_request_ctx(req);
>  	int ret;
>  
> +	if (!req->cryptlen)
> +		return 0;
> +
>  	if (unlikely(actx->key_len != AES_KEYSIZE_128 && !actx->key_referenced))
>  		return mxs_dcp_block_fallback(req, enc);

[Severity: High]
This isn't a bug introduced by this patch, but since this patch modifies
mxs_dcp_aes_enqueue(), could there be a deadlock here due to spin_lock()
usage?

The AES and SHA worker kthreads (dcp_chan_thread_aes and dcp_chan_thread_sha)
take the channel lock in process context:

drivers/crypto/mxs-dcp.c:dcp_chan_thread_aes() {
    ...
    spin_lock(&sdcp->lock[chan]);
    ...
}

However, the enqueue functions take the same lock without disabling softirqs:

drivers/crypto/mxs-dcp.c:mxs_dcp_aes_enqueue() {
    ...
    spin_lock(&sdcp->lock[actx->chan]);
    ret = crypto_enqueue_request(&sdcp->queue[actx->chan], &req->base);
    spin_unlock(&sdcp->lock[actx->chan]);
    ...
}

If a softirq (like IPsec network traffic) preempts the worker thread while the
lock is held, and attempts to enqueue a request, will it spin forever waiting
for the preempted kthread to release the lock? Should this use spin_lock_bh()
instead?

[Severity: High]
This is a pre-existing issue, but I noticed a potential out-of-bounds read
in mxs_dcp_aes_block_crypt() related to how scatterlist trailing data is
handled.

If a decryption request has a cryptlen that is a multiple of 4096 and the
scatterlist contains trailing data, the first 4096 bytes are processed,
making actx->fill = 4096. After submission, actx->fill becomes 0. For the
next element, limit_hit is true, and clen is 0.

Could this cause a 0-length hardware submission and an out-of-bounds read?

drivers/crypto/mxs-dcp.c:mxs_dcp_aes_block_crypt() {
    ...
    if (actx->fill == out_off || sg_is_last(src) || limit_hit) {
    ...
        last_out_len = actx->fill;
        actx->fill = 0;
    ...
        memcpy(req->iv, in_buf + (last_out_len - AES_BLOCK_SIZE),
               AES_BLOCK_SIZE);
    ...
}

With last_out_len = 0, does the memcpy underflow the pointer (0 - 16) and read
16 bytes before the in_buf array?

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential invalid
memory access in mxs_dcp_aes_block_crypt() when handling scatterlists.

The code extracts virtual addresses and DMA lengths directly from the unmapped
scatterlist:

drivers/crypto/mxs-dcp.c:mxs_dcp_aes_block_crypt() {
    ...
    src_buf = sg_virt(src);
    len = sg_dma_len(src);
    ...
}

Since the scatterlist is not mapped first, wouldn't sg_virt() return a NULL
pointer for HIGHMEM pages, leading to a panic during the subsequent memcpy?
Additionally, is it safe to read sg_dma_len() before a DMA mapping has
occurred, especially on platforms that require CONFIG_NEED_SG_DMA_LENGTH?

[Severity: High]
This is a pre-existing issue, but I see a risk of memory corruption in
mxs_dcp_start_dma() when handling timeouts.

When wait_for_completion_timeout() expires, the function simply returns:

drivers/crypto/mxs-dcp.c:mxs_dcp_start_dma() {
    ...
    if (!wait_for_completion_timeout(&sdcp->completion[chan],
                                     msecs_to_jiffies(1000)))
        return -ETIMEDOUT;
    ...
}

Since the active DMA transfer is abandoned without halting the hardware
channel, could the hardware resume the transaction later? If the caller unmaps
the buffers and the worker reuses the shared bounce buffers (aes_in_buf,
aes_out_buf) for the next request, wouldn't a delayed DMA completion overwrite
these buffers while they are in use?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908094437.1548278-1-lilinmao@kylinos.cn?part=1