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

New issues:
- [High] BAM DMA command descriptors misinterpret host-endian values on big-endian architectures, causing incorrect register writes.

Pre-existing issues:
- [High] The error cleanup path in `qce_aead_async_req_handle` unmaps the wrong scatterlist for the source buffer when `diff_dst` is true, leaking associated data mappings.
- [High] Incorrect error check for `dma_map_sg()` allows an unmapped scatterlist to be erroneously unmapped on fallback paths.
--

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

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

This commit switches the driver to use BAM DMA for register I/O instead of
direct register writes. It provides the necessary DMA infrastructure, updates
the ordering of operations, and replaces direct I/O calls with wrappers that
queue 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;

[Severity: High]
This is a pre-existing issue, but is the error check for dma_map_sg()
correct prior to this step?

Looking earlier in qce_aead_async_req_handle():

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

Since dma_map_sg() returns 0 on failure, checking for < 0 bypasses this error
handler. This allows an unmapped scatterlist to proceed and later be
erroneously unmapped on fallback paths (such as if qce_start or
qce_dma_prep_sgs fails).

[Severity: High]
This is also a pre-existing issue, but does this error path unmap the wrong
scatterlist when diff_dst is true?

In qce_aead_async_req_handle(), the source buffer is mapped using:

    src_nents = dma_map_sg(qce->dev, rctx->src_sg, rctx->src_nents, dir_src);

However, if a subsequent step fails and execution jumps to error_unmap_src,
the code performs:

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

Could this leak IOMMU mappings by unmapping req->src instead of the actually
mapped rctx->src_sg buffer?

[ ... ]

> diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
> index 26347e9fc078a..88d253d9147cf 100644
> --- a/drivers/crypto/qce/dma.c
> +++ b/drivers/crypto/qce/dma.c
> @@ -11,6 +13,98 @@
>  #include "dma.h"
>  
>  #define QCE_IGNORE_BUF_SZ		(2 * QCE_BAM_BURST_SIZE)
> +#define QCE_BAM_CMD_SGL_SIZE		128
[ ... ]
> +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 direct cast incorrectly handle host-endian values on big-endian
architectures? 

The buf parameter is passed a native u32 pointer from qce_write_dma():

    unsigned int reg_addr = ((unsigned int)(qce->base_phys) + offset);
    qce_prep_dma_cmd_desc(qce, &qce->dma, reg_addr, &val);

By casting the native u32 to *((__le32 *)buf) without applying cpu_to_le32(),
the little-endian BAM DMA hardware might receive reversed byte sequences for
crypto register writes on big-endian systems.

> +
> +	bam_ce_buf = &bam_txn->bam_ce[bam_txn->pre_bam_ce_idx];
> +	bam_txn->bam_ce_idx++;

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