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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch prevents an IV pointer underflow for 0-length skcipher requests, but the exact same underflow still occurs for certain non-zero length requests if the payload triggers a 0-byte hardware flush.
- [High] DMA mapping for `desc_phys` is leaked in `mxs_dcp_start_dma()` on hardware timeout or error paths.
--

commit d4942ea53ae872751299af5195d892f7af73d2e1
Author: Linmao Li <lilinmao@kylinos.cn>
crypto: mxs-dcp: handle zero-length skcipher requests

This patch returns success early for zero-length skcipher requests in
mxs_dcp_aes_enqueue() to prevent an invalid source access and IV pointer
underflow.

> 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;
> +

[Severity: High]
This is a pre-existing issue, but while this patch prevents an IV pointer
underflow for 0-length requests, does the exact same underflow still occur
for certain non-zero length requests if the payload triggers a 0-byte
hardware flush?

If req->cryptlen is a non-zero multiple of 4096 bytes and the scatterlist
contains trailing extra data, limit_hit evaluates to true. Because the data
perfectly filled the bounce buffer on the previous iteration, actx->fill was
reset to 0.

In mxs_dcp_aes_block_crypt(), the do-while loop would execute once with len
clamped to 0:

    if (limit_hit)
            len = req->cryptlen - (tlen - len);

    do {
            ...
            if (actx->fill == out_off || sg_is_last(src) ||
                limit_hit) {
                    ret = mxs_dcp_run_aes(actx, req, init);

This unconditionally submits the buffer with actx->fill == 0. Then
last_out_len is overwritten to 0:

            last_out_len = actx->fill;

Causing an out-of-bounds read when calculating the IV offset:

    memcpy(req->iv, out_buf+(last_out_len-AES_BLOCK_SIZE),
            AES_BLOCK_SIZE);

Could this allow a kernel oops or a leak of up to 16 bytes of adjacent slab
memory to userspace via the updated IV?

[Severity: High]
This is a pre-existing issue, but if the hardware rejects the 0-byte flush
caused by the edge case above, does mxs_dcp_start_dma() leak the DMA
mapping for desc_phys?

In mxs_dcp_start_dma(), if the channel times out or reports an error, it
returns without calling dma_unmap_single() for the descriptor:

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

    if (stat & 0xff) {
            dev_err(sdcp->dev, "Channel %i error (CH_STAT=0x%08x)\n",
                    chan, stat);
            return -EINVAL;
    }

Could this resource leak be triggered repeatedly by untrusted input to
exhaust IOMMU or swiotlb space, leading to a denial of service?

>  	if (unlikely(actx->key_len != AES_KEYSIZE_128 && !actx->key_referenced))
>  		return mxs_dcp_block_fallback(req, enc);

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