Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Use-After-Free during driver removal due to missing IRQ synchronization.
- [High] Sleeping in atomic context when freeing virtual DMA descriptors.
- [High] Out-of-bounds write in `aml_dma_prep_slave_sg` when processing zero-length scatterlist elements.
- [High] Broken pause/resume state machine logic leading to channel stall and state corruption.
- [Medium] `aml_dma_terminate_all()` fails to reset channel error state, causing permanent stall.
- [Medium] `aml_dma_tx_status()` improperly handles unissued descriptors, causing false errors and incorrect residues.
- [Medium] Mismatched DMA address registers used for 64-bit physical addresses.
--
commit 7102a7639af273fc6303ccde475117debb548165
Author: Xianwei Zhao <[email protected]>
dmaengine: amlogic: Add general DMA driver for A9
Amlogic A9 SoCs include a general-purpose DMA controller that can be used
by multiple peripherals, such as I2C PIO and I3C. Each peripheral group
is associated with a dedicated DMA channel in hardware.
> diff --git a/drivers/dma/amlogic-dma.c b/drivers/dma/amlogic-dma.c
> index 0000000000000..bce6932f3a12e
> --- /dev/null
> +++ b/drivers/dma/amlogic-dma.c
[ ... ]
> +static void aml_dma_free_desc(struct virt_dma_desc *vd)
> +{
> + struct aml_dma_desc *aml_desc = to_aml_dma_desc(vd);
> +
> + dma_free_coherent(aml_desc->dma_device->dev,
> + aml_desc->sg_link_size,
> + aml_desc->sg_link,
> + aml_desc->sg_link_phys);
[Severity: High]
Does this code sleep in atomic context?
When the virtual DMA descriptor is freed, vchan_dma_desc_free_list() calls
this callback from within vchan_complete(), which runs in tasklet
(softirq) context. Calling a sleepable function like dma_free_coherent()
from a tasklet can result in a scheduling while atomic panic.
Could this driver use dma_pool_alloc/free or kzalloc/kfree for descriptor
management instead?
> + kfree(aml_desc);
> +}
[ ... ]
> +/* DMA transfer state update how many data reside it */
> +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 || !txstate)
> + return ret;
> +
> + spin_lock_irqsave(&aml_chan->vchan.lock, flags);
> + vd = vchan_find_desc(&aml_chan->vchan, cookie);
> + 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) {
> + 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");
[Severity: Medium]
Will this incorrectly handle unissued descriptors?
Looking at aml_dma_tx_status(), it attempts to locate a descriptor using
vchan_find_desc(), which exclusively searches the desc_issued list.
If a client queries the status of a descriptor that was submitted (residing
in desc_submitted) but not yet issued via issue_pending(), vchan_find_desc()
will return NULL.
This causes the driver to log a spurious "cookie error" and return a residue
of 0, which might incorrectly imply the pending transfer is complete.
> + }
> + spin_unlock_irqrestore(&aml_chan->vchan.lock, flags);
[ ... ]
> +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)
> +{
> + 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 aml_dma_sg_link *sg_link = NULL;
> + struct scatterlist *sg = NULL;
> + u64 paddr;
> + u32 link_count, avail;
> + u32 i;
[ ... ]
> + sg_link = aml_desc->sg_link;
> + for_each_sg(sgl, sg, sg_len, i) {
> + avail = sg_dma_len(sg);
> + paddr = sg->dma_address;
> + 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);
> + paddr = paddr + SG_MAX_LEN;
> + avail = avail - SG_MAX_LEN;
> + sg_link++;
> + }
> + /* 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, avail);
> +
> + aml_desc->data_len += sg_dma_len(sg);
> + sg_link++;
[Severity: High]
Can this write out of bounds for zero-length scatterlist elements?
Earlier in aml_dma_prep_slave_sg(), sg_nents_for_dma() calculates the
required number of link descriptors. For an SG element with length 0, it
computes DIV_ROUND_UP(0, SG_MAX_LEN), adding 0 to the link count.
However, this for_each_sg loop unconditionally consumes at least one link
slot per SG element, setting its properties and incrementing sg_link.
If a client provides an empty SG element, this mismatch might result in an
out-of-bounds write past the end of the dma_alloc_coherent array, leading
to memory corruption.
> + }
[ ... ]
> +static int aml_dma_chan_pause(struct dma_chan *chan)
> +{
> + struct aml_dma_chan *aml_chan = to_aml_dma_chan(chan);
> + struct aml_dma_dev *aml_dma = aml_chan->aml_dma;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&aml_chan->vchan.lock, flags);
> + regmap_set_bits(aml_dma->regmap, aml_chan->reg_offs + RCH_CFG, CFG_PAUSE);
> + aml_chan->pre_status = aml_chan->status;
> + aml_chan->status = DMA_PAUSED;
[Severity: High]
Is the pause and resume state tracking prone to corruption?
If aml_dma_chan_pause() is called twice, it overwrites pre_status with
DMA_PAUSED, which might brick the channel upon resume.
Also, if a client submits a new transaction while paused,
aml_dma_chan_resume() restores DMA_COMPLETE but doesn't call
aml_dma_start(), which could permanently stall the channel.
> + spin_unlock_irqrestore(&aml_chan->vchan.lock, flags);
> +
> + return 0;
> +}
[ ... ]
> +static int aml_dma_terminate_all(struct dma_chan *chan)
> +{
> + struct aml_dma_chan *aml_chan = to_aml_dma_chan(chan);
> + struct aml_dma_dev *aml_dma = aml_chan->aml_dma;
> + int chan_id = aml_chan->chan_id;
> + struct virt_dma_desc *cur_vd;
> + unsigned long flags;
> + LIST_HEAD(head);
> +
> + spin_lock_irqsave(&aml_chan->vchan.lock, flags);
[Severity: Medium]
Will the channel stall permanently if terminated after errors?
When a channel hits 6 hardware errors, aml_dma_start() halts the channel
and abandons pending processing. To recover, a client driver typically calls
dmaengine_terminate_all().
However, aml_dma_terminate_all() doesn't reset aml_chan->status to
DMA_COMPLETE or aml_chan->err_num to 0. Any subsequent transactions
submitted to this channel might hang forever because aml_dma_start() will
immediately return due to the persistent error state.
> + regmap_set_bits(aml_dma->regmap, aml_chan->reg_offs + RCH_CFG, CFG_PAUSE);
[ ... ]
> +static void aml_dma_start(struct aml_dma_chan *aml_chan)
> +{
> + struct virt_dma_desc *vd = vchan_next_desc(&aml_chan->vchan);
> + struct aml_dma_dev *aml_dma = aml_chan->aml_dma;
> + struct aml_dma_desc *aml_desc = NULL;
> + int chan_id = aml_chan->chan_id;
[ ... ]
> + if (aml_chan->direction == DMA_MEM_TO_DEV) {
> + regmap_write(aml_dma->regmap, aml_chan->reg_offs + RCH_ADDR,
> + lower_32_bits(aml_desc->sg_link_phys));
> + regmap_write(aml_dma->regmap, aml_chan->reg_offs + RCH_ADDR_HIGH,
> + upper_32_bits(aml_desc->sg_link_phys));
[Severity: Medium]
Are the correct DMA address registers being used for 64-bit addresses?
The hardware registers define RCH_ADDR (0xc), RCH_ADDR_LOW (0x20), and
RCH_ADDR_HIGH (0x24).
In aml_dma_start(), the lower 32 bits of the physical address are programmed
into the legacy 32-bit RCH_ADDR (0xc) rather than RCH_ADDR_LOW (0x20), while
writing the upper 32 bits to RCH_ADDR_HIGH (0x24).
If the hardware expects the lower bits of a 64-bit address at 0x20, this
might result in an invalid address and immediate DMA failures.
> + regmap_write(aml_dma->regmap, aml_chan->reg_offs + RCH_LEN, aml_desc->data_len);
[ ... ]
> +static irqreturn_t aml_dma_interrupt_handler(int irq, void *dev_id)
> +{
> + struct aml_dma_dev *aml_dma = dev_id;
> + struct aml_dma_chan *aml_chan;
> + struct aml_dma_desc *aml_desc;
> + u32 done, eoc_done, err, err_l, end;
> + u32 cpl_data;
> + int i = 0;
[ ... ]
> + while (done) {
> + i = ffs(done) - 1;
> + regmap_write(aml_dma->regmap, CLEAR_RCH, BIT(i));
> + done &= ~BIT(i);
> + aml_chan = aml_dma->aml_rch[i];
> + if (!aml_chan) {
> + dev_err(aml_dma->dma_device.dev, "idx %d rch not initialized\n", i);
> + continue;
> + }
> + spin_lock(&aml_chan->vchan.lock);
> + aml_chan->status = (err & BIT(i)) ? DMA_ERROR : DMA_COMPLETE;
[Severity: High]
Can an interrupt overwrite the paused state?
If an interrupt fires while the channel is paused, this handler unconditionally
overwrites status to DMA_COMPLETE and calls aml_dma_start(). Could this unpause
the hardware and break the pause semantics?
> + aml_desc = aml_chan->cur_desc;
[ ... ]
> +static int aml_dma_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct dma_device *dma_dev;
> + struct aml_dma_dev *aml_dma;
> + int ret, i, len;
> + u32 chan_nr;
[ ... ]
> + ret = devm_request_irq(&pdev->dev, aml_dma->irq, aml_dma_interrupt_handler,
> + 0, dev_name(&pdev->dev), aml_dma);
[Severity: High]
Can this cause a use-after-free during driver removal?
Since devm_request_irq() is used in aml_dma_probe(), the IRQ remains active
until after aml_dma_remove() completes.
In aml_dma_remove(), the driver loops and calls tasklet_kill(&aml_chan->vchan.task):
> +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);
But an executing interrupt handler on another CPU can subsequently call
vchan_cookie_complete() and reschedule the tasklet.
Once remove() returns, devres unregisters the DMA engine, frees the IRQ, and
frees the driver struct via devm_kfree. The tasklet will then run and access
freed memory, causing a use-after-free panic.
Should the driver explicitly free or disable the IRQ using devm_free_irq()
before calling tasklet_kill()?
> + aml_chan = aml_dma->aml_wch[i];
> + if (aml_chan)
> + tasklet_kill(&aml_chan->vchan.task);
> + }
> +}
--
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
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.