Re: [PATCH v21 08/14] crypto: qce - Include algapi.h in the core.h header

[sashiko-bot@kernel.org]

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

New issues:
- [High] bam_make_lock_desc maps the command descriptor using the slave device pointer instead of the DMA controller device, which will cause IOMMU faults or unresolvable physical addresses.
- [High] LIFO devres execution order in qce_crypto_probe causes the DMA channels to be freed before the workqueue is cancelled, leading to use-after-free if the workqueue executes concurrently.
- [High] BAM pipe locking is restricted to TX channels (Memory-to-Device), which directly contradicts the commit message stating it applies to RX channels.

Pre-existing issues:
- [High] Writing to BAM registers after forcefully suspending the device causes an unclocked access crash.
--

--- Patch [8]: [PATCH v21 08/14] crypto: qce - Include algapi.h in the core.h header ---
commit 2564fb6b194c2eaa4eed81579eda988d138343a5
Author: Bartosz Golaszewski <brgl@kernel.org>

crypto: qce - Include algapi.h in the core.h header

The header defines a struct embedding struct crypto_queue whose size
needs to be known and which is defined in crypto/algapi.h. Move the
inclusion from core.c to core.h.

> diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
> index ac74f69914d61..dd860435d2c47 100644
> --- a/drivers/crypto/qce/core.c
> +++ b/drivers/crypto/qce/core.c

[ ... ]

> @@ -226,6 +232,11 @@ static int qce_crypto_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	INIT_WORK(&qce->done_work, qce_req_done_work);
> +	ret = devm_add_action_or_reset(dev, qce_cancel_work, &qce->done_work);

[Severity: High]
Does registering the workqueue cancellation here cause a use-after-free
during device teardown?

Since devres actions are executed in reverse order (LIFO),
devm_qce_dma_request() registers qce_dma_release() which will run first
and release the DMA channels.

Only after that does qce_cancel_work() execute, meaning if
qce_req_done_work() runs concurrently during this window, it could
access the already freed DMA channels.

> +	if (ret)
> +		return ret;
> +
>  	ret = devm_qce_dma_request(qce->dev, &qce->dma);
>  	if (ret)
>  		return ret;

[ ... ]

> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index 1bb26af0405f3..f08549ee3872e 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c

[ ... ]

> @@ -1021,13 +1113,102 @@ static void bam_apply_new_config(struct bam_chan *bchan,
>  	bchan->reconfigure = 0;
>  }
>  
> +static struct bam_async_desc *
> +bam_make_lock_desc(struct bam_chan *bchan, unsigned long flag)
> +{
> +	struct dma_chan *chan = &bchan->vc.chan;
> +	struct bam_async_desc *async_desc;
> +	struct bam_desc_hw *desc;
> +	struct virt_dma_desc *vd;
> +	struct virt_dma_chan *vc;
> +	unsigned int mapped;
> +
> +	async_desc = kzalloc_flex(*async_desc, desc, 1, GFP_NOWAIT);
> +	if (!async_desc) {
> +		dev_err(bchan->bdev->dev, "failed to allocate the BAM lock descriptor\n");
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	sg_init_table(&async_desc->lock_sg, 1);
> +
> +	async_desc->num_desc = 1;
> +	async_desc->curr_desc = async_desc->desc;
> +	async_desc->dir = DMA_MEM_TO_DEV;
> +	async_desc->is_lock_desc = true;
> +
> +	desc = async_desc->desc;
> +
> +	bam_prep_ce_le32(&async_desc->lock_ce, bchan->scratchpad_addr, BAM_WRITE_COMMAND, 0);
> +	sg_set_buf(&async_desc->lock_sg, &async_desc->lock_ce, sizeof(async_desc->lock_ce));
> +
> +	mapped = dma_map_sg(chan->slave, &async_desc->lock_sg, 1, DMA_TO_DEVICE);

[Severity: High]
Does mapping the command descriptor using chan->slave instead of the DMA
controller device cause IOMMU faults?

The memory is mapped into the peripheral's IOMMU domain rather than the
DMA controller's domain, which could lead to DMA faults when the controller
tries to read the descriptor.

> +	if (!mapped) {
> +		kfree(async_desc);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	desc->flags |= cpu_to_le16(DESC_FLAG_CMD | flag);
> +	desc->addr = sg_dma_address(&async_desc->lock_sg);
> +	desc->size = cpu_to_le16(sizeof(struct bam_cmd_element));

[ ... ]

> +static int bam_setup_pipe_lock(struct bam_chan *bchan)
> +{
> +	const struct bam_device_data *bdata = bchan->bdev->dev_data;
> +	struct bam_async_desc *lock_desc, *unlock_desc;
> +
> +	lockdep_assert_held(&bchan->vc.lock);
> +
> +	if (!bdata->pipe_lock_supported || !bchan->scratchpad_addr ||
> +	    bchan->direction != DMA_MEM_TO_DEV)

[Severity: High]
Does restricting this hardware locking workaround to DMA_MEM_TO_DEV (TX)
channels contradict the intent to apply it to RX channels?

The commit message states "when starting DMA on an RX channel - prepend
the existing queue...", but this check prevents the locking from applying
to DMA_DEV_TO_MEM (RX) channels.

> +		return 0;
> +
> +	/*
> +	 * Allocate both the LOCK and the UNLOCK descriptors up-front so the
> +	 * operation is all-or-nothing: if either allocation fails we free both

[ ... ]

> @@ -1401,6 +1603,8 @@ static void bam_dma_remove(struct platform_device *pdev)
>  	struct bam_device *bdev = platform_get_drvdata(pdev);
>  	u32 i;
>  
> +	free_irq(bdev->irq, bdev);
> +
>  	pm_runtime_force_suspend(&pdev->dev);
>  
>  	of_dma_controller_free(pdev->dev.of_node);
> @@ -1409,8 +1613,6 @@ static void bam_dma_remove(struct platform_device *pdev)
>  	/* mask all interrupts for this execution environment */
>  	writel_relaxed(0, bam_addr(bdev, 0,  BAM_IRQ_SRCS_MSK_EE));

[Severity: High]
This is a pre-existing issue, but does writing to BAM_IRQ_SRCS_MSK_EE after
pm_runtime_force_suspend() has disabled the clock cause a synchronous
external abort?

Since the clock is turned off by the suspend callback, this unclocked access
can trigger a fatal bus error.

> -	devm_free_irq(bdev->dev, bdev->irq, bdev);
> -
>  	for (i = 0; i < bdev->num_channels; i++) {
>  		bam_dma_terminate_all(&bdev->channels[i].vc.chan);
>  		tasklet_kill(&bdev->channels[i].vc.task);

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