Re: [PATCH v11 2/3] dmaengine: amlogic: Add general DMA driver for A9

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] aml_dma_tx_status falsely reports in-flight transfers as DMA_COMPLETE, causing data corruption.
- [High] Data race and TOCTOU NULL pointer dereference in aml_dma_tx_status due to early lock release.
- [High] Use-After-Free in devres-managed remove path due to tasklet scheduled after tasklet_kill.
- [High] Pending free_work items on the global workqueue cause a kernel panic on module unload.
- [High] aml_dma_terminate_all() masks interrupts but does not clear pending interrupt status, potentially corrupting subsequent transfers.
- [Medium] Missing src_addr_widths configuration prevents DMA_DEV_TO_MEM clients from validating capabilities.
- [Medium] DMA descriptors use host-endian fields, breaking hardware functionality on big-endian platforms.
--

--- Patch [2]: [PATCH v11 2/3] dmaengine: amlogic: Add general DMA driver for A9 ---
commit 91d4c9c09e9b3baa0931dae1e37c022fae2b7447
Author: Xianwei Zhao <[email protected]>

dmaengine: amlogic: Add general DMA driver for A9

This patch introduces a new DMA engine driver for Amlogic A9 SoCs. The
general-purpose DMA controller is used by multiple peripherals such as I2C,
PIO, and I3C.

> diff --git a/drivers/dma/amlogic-dma.c b/drivers/dma/amlogic-dma.c
> new file mode 100644
> index 0000000000000..9de650a79abac
> --- /dev/null
> +++ b/drivers/dma/amlogic-dma.c

[ ... ]

> +struct aml_dma_sg_link {
> +#define LINK_LEN		GENMASK(26, 0)
> +#define LINK_IRQ		BIT(27)
> +#define LINK_EOC		BIT(28)
> +#define LINK_LOOP		BIT(29)
> +#define LINK_ERR		BIT(30)
> +#define LINK_OWNER		BIT(31)
> +	u32 ctl;
> +	u32 addr_low;
> +	u32 addr_high;
> +	u32 revered;
> +} __packed;

[Severity: Medium]
Since this structure describes a hardware memory descriptor, should these
fields use __le32 instead of u32 to ensure the driver functions correctly on
big-endian architectures?

[ ... ]

> +static void aml_dma_free_desc(struct virt_dma_desc *vd)
> +{
> +	struct aml_dma_desc *aml_desc = to_aml_dma_desc(vd);
> +
> +	schedule_work(&aml_desc->free_work);
> +}

[Severity: High]
Does scheduling memory cleanup on the system workqueue here introduce a panic
on module unload?

If the driver module is unloaded while a work item is still pending, the
module's text segment will be unmapped. When the workqueue executes
aml_dma_free_desc_work(), it might jump to unmapped memory because
aml_dma_remove() does not appear to flush this work.

[ ... ]

> +static enum dma_status aml_dma_tx_status(struct dma_chan *chan,
> +					 dma_cookie_t cookie,
> +					 struct dma_tx_state *txstate)
> +{
> +	struct aml_dma_chan *aml_chan = to_aml_dma_chan(chan);
> +	struct aml_dma_dev *aml_dma = aml_chan->aml_dma;
> +	struct aml_dma_desc *aml_desc = NULL;
> +	struct virt_dma_desc *vd;
> +	u32 residue = 0, done;
> +	unsigned long flags;
> +	enum dma_status ret;
> +
> +	ret = dma_cookie_status(chan, cookie, txstate);
> +	if (ret == DMA_COMPLETE)
> +		return ret;
> +
> +	ret = aml_chan->status;

[Severity: High]
Could this overwrite the correct DMA_IN_PROGRESS return value from
dma_cookie_status() with aml_chan->status?

If aml_chan->status is usually set to DMA_COMPLETE during initialization,
polling clients might mistakenly believe the transfer is finished before it
actually is, leading to data corruption.

> +	if (!txstate)
> +		return ret;
> +
> +	spin_lock_irqsave(&aml_chan->vchan.lock, flags);
> +	vd = vchan_find_desc(&aml_chan->vchan, cookie);
> +	spin_unlock_irqrestore(&aml_chan->vchan.lock, flags);
> +	if (vd) {
> +		aml_desc = to_aml_dma_desc(vd);
> +		residue = aml_desc->data_len;
> +	} else if (aml_chan->cur_desc && aml_chan->cur_desc->vd.tx.cookie == cookie) {

[Severity: High]
Can releasing the lock before accessing aml_chan->cur_desc lead to a
NULL pointer dereference?

If a hardware interrupt completes the transfer concurrently, it sets cur_desc
to NULL. A thread executing this code could check aml_chan->cur_desc (finding
it non-NULL), but if the interrupt handler clears the pointer right after,
the subsequent aml_chan->cur_desc->vd.tx.cookie access would dereference NULL.

Alternatively, aml_desc could be assigned NULL, causing a dereference when
calculating the residue.

> +		aml_desc = aml_chan->cur_desc;
> +		regmap_read(aml_dma->regmap, aml_chan->reg_offs + RCH_RD_LEN, &done);
> +		residue = aml_desc->data_len - done;
> +	} else {
> +		dev_err(aml_dma->dma_device.dev, "cookie error\n");
> +	}
> +	dma_set_residue(txstate, residue);
> +
> +	return ret;
> +}

[ ... ]

> +static struct dma_async_tx_descriptor *aml_dma_prep_slave_sg
> +		(struct dma_chan *chan, struct scatterlist *sgl,
> +		unsigned int sg_len, enum dma_transfer_direction direction,
> +		unsigned long flags, void *context)
> +{

[ ... ]

> +		while (avail > SG_MAX_LEN) {
> +			/* set dma address and len to sglink*/
> +			sg_link->addr_low = lower_32_bits(paddr);
> +			sg_link->addr_high = upper_32_bits(paddr);
> +			sg_link->ctl = FIELD_PREP(LINK_LEN, SG_MAX_LEN);

[Severity: Medium]
Should these assignments use cpu_to_le32() to ensure proper conversion of
host-endian values before writing to coherent DMA memory? Without it, this
could break hardware functionality on big-endian platforms.

[ ... ]

> +static int aml_dma_terminate_all(struct dma_chan *chan)
> +{

[ ... ]

> +	spin_lock_irqsave(&aml_chan->vchan.lock, flags);
> +	regmap_set_bits(aml_dma->regmap, aml_chan->reg_offs + RCH_CFG, CFG_PAUSE);
> +	regmap_set_bits(aml_dma->regmap, aml_chan->reg_offs + RCH_CFG, CFG_CLEAR);
> +
> +	if (aml_chan->direction == DMA_MEM_TO_DEV)
> +		regmap_set_bits(aml_dma->regmap, RCH_INT_MASK, BIT(chan_id));
> +	else if (aml_chan->direction == DMA_DEV_TO_MEM)
> +		regmap_set_bits(aml_dma->regmap, WCH_INT_MASK, BIT(chan_id));

[Severity: High]
Does masking the interrupts here without also clearing the pending interrupt
status (via CLEAR_RCH or CLEAR_WCH) risk corrupting subsequent transfers?

If an interrupt is already pending, its status bit remains set. When
aml_dma_start() is called later and unmasks the interrupt, it will fire
immediately and falsely complete the new transfer.

[ ... ]

> +static int aml_dma_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	dma_dev->device_issue_pending = aml_dma_issue_pending;
> +	/* PIO 4 bytes and I2C 1 byte */
> +	dma_dev->dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) | BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
> +	dma_dev->directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);

[Severity: Medium]
Since this driver advertises DMA_DEV_TO_MEM support, is there a missing
configuration for src_addr_widths?

If omitted, client drivers validating source widths using
dma_get_slave_caps() will see a width of 0 and might fail initialization.

[ ... ]

> +static void aml_dma_remove(struct platform_device *pdev)
> +{
> +	struct aml_dma_dev *aml_dma = platform_get_drvdata(pdev);
> +	struct aml_dma_chan *aml_chan = NULL;
> +	int i;
> +
> +	of_dma_controller_free((&pdev->dev)->of_node);
> +
> +	regmap_write(aml_dma->regmap, RCH_INT_MASK, 0xffffffff);
> +	regmap_write(aml_dma->regmap, WCH_INT_MASK, 0xffffffff);
> +
> +	for (i = 0; i < MAX_CHAN_ID; i++) {
> +		aml_chan = aml_dma->aml_rch[i];
> +		if (aml_chan)
> +			tasklet_kill(&aml_chan->vchan.task);
> +		aml_chan = aml_dma->aml_wch[i];
> +		if (aml_chan)
> +			tasklet_kill(&aml_chan->vchan.task);
> +	}
> +}

[Severity: High]
Can a use-after-free occur during driver unbind if an interrupt fires
concurrently?

Since the devres-managed IRQ is not freed or synchronized before calling
tasklet_kill(), an active interrupt handler on another CPU could schedule the
tasklet again right after tasklet_kill() returns. Once the remove function
finishes, devres frees the channel memory containing the tasklet, leading to
execution on freed memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic