Re: [PATCH v20 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 4 potential issue(s) to consider:

New issues:
- [High] Incorrect endianness conversion for BAM DMA register writes on big-endian architectures.
- [Medium] DMA descriptor leak in qce_submit_cmd_desc() on error paths.

Pre-existing issues:
- [High] Memory allocated for rctx->adata is never freed.
- [High] Incorrect dma_map_sg failure check for src_sg ignores mapping errors.
--

commit bf3a155521eb3078dcc0111460444f36ebb69ebf
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
> --- 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)

[Severity: High]
This is a pre-existing issue, but does this code leak the rctx->adata buffer
allocated for AES-CCM operations? It looks like if memory is dynamically
allocated for rctx->adata via kzalloc() during CCM processing, it is never
freed in either the success completion path or error handling paths of
qce_aead_async_req_handle(), which could lead to memory exhaustion when
processing untrusted network traffic.

[Severity: High]
This is also a pre-existing issue, but does the error check for dma_map_sg()
earlier in qce_aead_async_req_handle() correctly handle mapping failures?
dma_map_sg() returns 0 on error, not a negative value. The check for
(src_nents < 0) evaluates to false, which means a mapping failure could be
ignored, causing the driver to proceed and later call dma_unmap_sg() with
invalid parameters.

[ ... ]

> 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

[ ... ]

> +int qce_submit_cmd_desc(struct qce_device *qce)
> +{
[ ... ]
> +	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]
Does this error path leak the unsubmitted DMA descriptor? If the DMA
submission fails and we go to err_unmap_sg, the rollback bypasses
qce_dma_terminate_all() or any mechanism to flush the descriptor. Since the
DMA_CTRL_REUSE flag was not passed during preparation, attempting to explicitly
free it later will return an error without freeing the descriptor.

[ ... ]

> +static void qce_prep_dma_cmd_desc(struct qce_device *qce, struct qce_dma_data *dma,
> +				  unsigned int addr, void *buf)
> +{
[ ... ]
> +	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));
[ ... ]
> +void qce_write_dma(struct qce_device *qce, unsigned int offset, u32 val)
> +{
> +	unsigned int reg_addr = ((unsigned int)(qce->base_phys) + offset);
> +
> +	qce_prep_dma_cmd_desc(qce, &qce->dma, reg_addr, &val);
> +}

[Severity: High]
Is there an endianness mismatch here on big-endian architectures?
In qce_write_dma(), a CPU-endian u32 stack address is passed to
qce_prep_dma_cmd_desc(). That function then casts the pointer to __le32* and
reads it directly into the BAM descriptor without byte swapping. Will this
result in reversed bytes being written to the crypto hardware registers on
big-endian CPUs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629-qcom-qce-cmd-descr-v20-0-56f67da84c05@oss.qualcomm.com?part=13