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

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

New issues:
- [High] Type punning of host-endian `u32` to `__le32` causes BAM DMA to write byte-swapped values to configuration registers on big-endian architectures.
- [Medium] Premature error return in `qce_dma_terminate_all()` leads to a leaked DMA mapping and a skipped TX channel termination.

Pre-existing issues:
- [High] The error path `error_unmap_src` unmaps `req->src`, but for CCM algorithms with associated data, the DMA mapping was actually performed on a newly allocated scatterlist table `rctx->src_sg`.
--

--- Patch [13]: [PATCH v24 13/14] crypto: qce - Add BAM DMA support for crypto register I/O ---
commit 4d2a36eb6d9e1e8c0d5c28a8810d39001454e83e
Author: Bartosz Golaszewski <[email protected]>
crypto: qce - Add BAM DMA support for crypto register I/O

This commit switches the driver to use BAM DMA for register I/O alongside
data transfer. It introduces infrastructure to queue DMA command descriptors
for register writes, altering the sequence of operations accordingly.

> diff --git a/drivers/crypto/qce/aead.c b/drivers/crypto/qce/aead.c
> --- a/drivers/crypto/qce/aead.c
> +++ b/drivers/crypto/qce/aead.c
> @@ -463,17 +463,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 the error handling in
qce_aead_async_req_handle() unmap the wrong scatterlist?

If a hardware setup failure or other error occurs during CCM request
processing with associated data, the error path drops into error_unmap_src:

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

Earlier in the function, qce_aead_ccm_prepare_buf() constructs a new
scatterlist table and assigns rctx->src_sg = rctx->src_tbl.sgl.

If dma_map_sg() mapped rctx->src_sg, wouldn't unmapping req->src instead
cause a memory leak and potentially trigger IOMMU corruption or DMA API
warnings?

> diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
> --- a/drivers/crypto/qce/dma.c
> +++ b/drivers/crypto/qce/dma.c
[ ... ]
> +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: High]
Does this code write byte-swapped values to the configuration registers
on big-endian architectures?

In qce_write_dma(), the host-endian u32 val is passed by reference:

    qce_prep_dma_cmd_desc(qce, &qce->dma, reg_addr, &val);

Inside qce_prep_dma_cmd_desc(), it is cast directly to a little-endian
pointer and dereferenced without byte-swapping, as shown above.

Could this cause the BAM DMA controller to interpret the data backwards on
big-endian systems? It seems cpu_to_le32() might be needed here.

[ ... ]
> @@ -98,28 +202,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]
Can this early return leak the DMA mapping and leave the TX channel active?

If dmaengine_terminate_all(dma->rxchan) fails and returns an error, the
function returns immediately. This bypasses the newly added dma_unmap_sg()
cleanup for the command descriptors:

    dma_unmap_sg(qce->dev, bam_txn->wr_sgl, bam_txn->wr_sgl_cnt, DMA_TO_DEVICE);

It also skips 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/[email protected]?part=13