Re: [PATCH v21 13/14] crypto: qce - Add BAM DMA support for crypto register I/O

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] BAM DMA command descriptors are written with native host-endian `u32` values, causing byte-swapping data corruption on big-endian platforms.
- [Medium] Early return in `qce_dma_terminate_all()` leaks DMA mappings and skips terminating the TX channel if RX channel termination fails.
- [Medium] Leaked DMA descriptor in error path due to incorrect use of `dmaengine_desc_free` on a non-reusable descriptor.

Pre-existing issues:
- [High] Memory leak of `rctx->adata` buffer in CCM requests with associated data on both success and error paths.
- [High] The error path in `qce_aead_async_req_handle` unmaps the wrong scatterlist for CCM requests, leading to a DMA mapping leak and invalid unmap.
- [High] Failure of `dma_map_sg` for the source scatterlist is bypassed due to an incorrect error check, leading to an invalid unmap.
--

--- Patch [13]: [PATCH v21 13/14] crypto: qce - Add BAM DMA support for crypto register I/O ---
commit 07a714be1f94a3c7b4d4c5c79104848b7570dad5
Author: Bartosz Golaszewski <brgl@kernel.org>

crypto: qce - Add BAM DMA support for crypto register I/O

Switch to using BAM DMA for register I/O in addition to passing data. To
that end: provide the necessary infrastructure in the driver, modify the
ordering of operations as required and replace all direct register writes
with wrappers queueing DMA command descriptors.

> diff --git a/drivers/crypto/qce/aead.c b/drivers/crypto/qce/aead.c
> index 92d84941d3dbf..48ea7fb9a91e6 100644
> --- a/drivers/crypto/qce/aead.c
> +++ b/drivers/crypto/qce/aead.c
[ ... ]
> @@ -461,17 +461,17 @@ qce_aead_async_req_handle(struct crypto_async_request *async_req)
>  			src_nents = dst_nents - 1;
>  	}
>  
> -	ret = qce_dma_prep_sgs(&qce->dma, rctx->src_sg, src_nents, rctx->dst_sg, dst_nents,
> -			       qce_aead_done, async_req);
> +	ret = qce_start(async_req, tmpl->crypto_alg_type);
>  	if (ret)
>  		goto error_unmap_src;
>  
> -	qce_dma_issue_pending(&qce->dma);
> -
> -	ret = qce_start(async_req, tmpl->crypto_alg_type);
> +	ret = qce_dma_prep_sgs(&qce->dma, rctx->src_sg, src_nents, rctx->dst_sg, dst_nents,
> +			       qce_aead_done, async_req);
>  	if (ret)
>  		goto error_terminate;
>  
> +	qce_dma_issue_pending(&qce->dma);
> +
>  	return 0;
>  
>  error_terminate:

[Severity: High]
This is a pre-existing issue, but does this function leak memory allocated
for CCM requests with associated data?

qce_aead_async_req_handle() calls qce_aead_ccm_prepare_buf() which
allocates:

    rctx->adata = kzalloc(...)

The buffer allocated for rctx->adata is never freed anywhere in the driver,
neither in the success path (qce_aead_done) nor in the error paths. Will
this reliably exhaust kernel memory for every CCM request?

[Severity: High]
This is a pre-existing issue, but is the wrong scatterlist being unmapped
in the error path?

If an error occurs, the error handler incorrectly unmaps the original user
scatterlist:

    error_unmap_src:
        if (diff_dst)
            dma_unmap_sg(qce->dev, req->src, rctx->src_nents, dir_src);

For CCM requests with associated data, the dynamically constructed
scatterlist rctx->src_sg is mapped. Does unmapping req->src instead cause a
permanent DMA mapping leak and trigger IOMMU faults?

[Severity: High]
This is a pre-existing issue, but does this function incorrectly check the
return value of dma_map_sg()?

Earlier in this function:

    src_nents = dma_map_sg(qce->dev, rctx->src_sg, rctx->src_nents,
                           dir_src);
    if (src_nents < 0) {

dma_map_sg() returns 0 on failure, not a negative value. Will this bypass
the error check, pass a count of 0 to qce_dma_prep_sgs(), and eventually
lead to an invalid unmap?

> diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
> index 26347e9fc078a..1b43c56503334 100644
> --- a/drivers/crypto/qce/dma.c
> +++ b/drivers/crypto/qce/dma.c
[ ... ]
> +int qce_submit_cmd_desc(struct qce_device *qce)
> +{
> +	struct qce_desc_info *qce_desc = qce->dma.bam_txn->desc;
> +	struct qce_bam_transaction *bam_txn = qce->dma.bam_txn;
> +	struct dma_async_tx_descriptor *dma_desc;
> +	struct dma_chan *chan = qce->dma.rxchan;
> +	unsigned long attrs = DMA_PREP_CMD;
> +	dma_cookie_t cookie;
> +	unsigned int mapped;
> +	int ret;
> +
> +	mapped = dma_map_sg(qce->dev, bam_txn->wr_sgl, bam_txn->wr_sgl_cnt, DMA_TO_DEVICE);
> +	if (!mapped)
> +		return -ENOMEM;
> +
> +	dma_desc = dmaengine_prep_slave_sg(chan, bam_txn->wr_sgl, mapped, DMA_MEM_TO_DEV, attrs);
> +	if (!dma_desc) {
> +		ret = -ENOMEM;
> +		goto err_unmap_sg;
> +	}
> +
> +	qce_desc->dma_desc = dma_desc;
> +	cookie = dmaengine_submit(qce_desc->dma_desc);
> +
> +	ret = dma_submit_error(cookie);
> +	if (ret)
> +		goto err_unmap_sg;
> +
> +	return 0;
> +
> +err_unmap_sg:
> +	dma_unmap_sg(qce->dev, bam_txn->wr_sgl, bam_txn->wr_sgl_cnt, DMA_TO_DEVICE);
> +	return ret;
> +}

[Severity: Medium]
Is there a leaked DMA descriptor on this error path?

If dmaengine_desc_free(dma_desc) is ever used on error paths here (e.g., if
metadata attachment or submission fails):

    err_free_desc:
        dmaengine_desc_free(dma_desc);

Is it safe to call this? This API does not free non-reusable descriptors,
and returns -EPERM instead, leaving the descriptor permanently leaked in
the unsubmitted pool.

> +static void qce_prep_dma_cmd_desc(struct qce_device *qce, struct qce_dma_data *dma,
> +				  unsigned int addr, void *buf)
> +{
> +	struct qce_bam_transaction *bam_txn = dma->bam_txn;
> +	struct bam_cmd_element *bam_ce_buf;
> +	int bam_ce_size, cnt, idx;
> +
> +	idx = bam_txn->bam_ce_idx;
> +	bam_ce_buf = &bam_txn->bam_ce[idx];
> +	bam_prep_ce_le32(bam_ce_buf, addr, BAM_WRITE_COMMAND, *((__le32 *)buf));

[Severity: Medium]
Will this cause endianness issues on big-endian platforms?

In qce_write_dma(), a native host-endian u32 value is passed by reference:

    qce_write_dma()
       unsigned int reg_addr = ...
       qce_prep_dma_cmd_desc(qce, &qce->dma, reg_addr, &val);

Here, the pointer is cast to __le32 * and dereferenced without using
cpu_to_le32(). Won't the BAM DMA hardware receive incorrectly byte-swapped
values?

[ ... ]
> @@ -98,28 +200,36 @@ int qce_dma_prep_sgs(struct qce_dma_data *dma, struct scatterlist *rx_sg,
[ ... ]
>  int qce_dma_terminate_all(struct qce_dma_data *dma)
>  {
> +	struct qce_device *qce = container_of(dma, struct qce_device, dma);
> +	struct qce_bam_transaction *bam_txn = dma->bam_txn;
>  	int ret;
>  
>  	ret = dmaengine_terminate_all(dma->rxchan);
> -	return ret ?: dmaengine_terminate_all(dma->txchan);
> +	if (ret)
> +		return ret;

[Severity: Medium]
If dmaengine_terminate_all(dma->rxchan) returns an error, the function
returns immediately. Does this early return leak the DMA mappings for
bam_txn->wr_sgl and skip terminating the TX channel entirely?

> +
> +	dma_unmap_sg(qce->dev, bam_txn->wr_sgl, bam_txn->wr_sgl_cnt, DMA_TO_DEVICE);
> +
> +	return dmaengine_terminate_all(dma->txchan);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-qcom-qce-cmd-descr-v21-0-bc2583e18475@oss.qualcomm.com?part=13