Re: [PATCH v2 59/64] dmaengine: tegra186-gpc: use dma_chan_kill_bh

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

Pre-existing issues:
- [Critical] Zero-length DMA requests cause an integer underflow when calculating the hardware word count, resulting in an unbounded 4GB DMA transfer.
- [High] TOCTOU race condition in the interrupt handler `tegra_dma_isr` leading to a Use-After-Free of `dma_desc`.
- [High] Incorrect teardown order in `tegra_dma_free_chan_resources()` allows late interrupts to reschedule the channel bottom-half, leading to a Use-After-Free.
- [High] Ignored error return from `tegra_dma_terminate_all()` leads to descriptor memory leak and active hardware DMA during channel teardown.
- [High] An unsupported DMA slave bus width causes `get_transfer_param()` to silently propagate an error code (-EINVAL) into the DMA hardware's MMIO sequence register.
--

--- Patch [59]: [PATCH v2 59/64] dmaengine: tegra186-gpc: use dma_chan_kill_bh ---
commit a9e2657d46f0ef7a89cb50bf6bca1e73ae39ee11
Author: Allen Pais <[email protected]>

dmaengine: tegra186-gpc: use dma_chan_kill_bh
    
virt-dma now dispatches completion callbacks through per-channel BH
work instead of its tasklet. Cancel that work during teardown before
channel storage is released.

> diff --git a/drivers/dma/tegra186-gpc-dma.c b/drivers/dma/tegra186-gpc-dma.c
> index 64cedef1050ae..1ba8897ec614e 100644
> --- a/drivers/dma/tegra186-gpc-dma.c
> +++ b/drivers/dma/tegra186-gpc-dma.c
> @@ -1284,7 +1284,7 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
>  	tegra_dma_terminate_all(dc);

[Severity: High]
This isn't a bug introduced by this patch, but should we check the return
value of tegra_dma_terminate_all() here?

If tegra_dma_terminate_all() fails due to a hardware timeout, it returns early
without moving the active descriptor to the terminated list and without
disabling the hardware.

Since the caller ignores the return value, vchan_free_chan_resources() might
fail to locate and free the active descriptor. Could this leave the hardware
executing a transfer into client memory that may be subsequently freed?

>  	synchronize_irq(tdc->irq);
>  
> -	tasklet_kill(&tdc->vc.task);
> +	dma_chan_kill_bh(&tdc->vc.chan);

[Severity: High]
This is a pre-existing issue, but does killing the bottom-half before freeing
the IRQ expose a race window?

If the DMA hardware remains active after a timeout, and dma_chan_kill_bh() is
called before free_irq(), could a late interrupt fire and invoke
vchan_cookie_complete() or vchan_cyclic_callback()?

This would re-queue the bottom-half work. After vchan_free_chan_resources()
subsequently frees all descriptors, the queued bottom-half might execute and
dereference the freed memory.

>  	tdc->config_init = false;
>  	tdc->slave_id = -1;
>  	tdc->sid_dir = DMA_TRANS_NONE;


While reviewing the file, I noticed a few other pre-existing issues:

[Severity: High]
This isn't a bug introduced by this patch, but does tegra_dma_isr() have a
race condition leading to a use-after-free of dma_desc?

drivers/dma/tegra186-gpc-dma.c:tegra_dma_isr() {
    ...
    struct tegra_dma_desc *dma_desc = tdc->dma_desc;
    ...
    spin_lock_irqsave(&tdc->vc.lock, flags);
    ...
    if (!dma_desc)
        goto irq_done;
    ...
}

The shared descriptor pointer is cached into a local variable before acquiring
the spinlock.

If the ISR is delayed while another CPU calls tegra_dma_terminate_all(), the
descriptor is freed. Could a subsequent DMA transfer quickly trigger another
interrupt, causing the delayed ISR to acquire the lock, pass the check, and
dereference the stale local pointer?


[Severity: High]
This is a pre-existing issue, but do zero-length DMA requests cause an
integer underflow when calculating the hardware word count?

In tegra_dma_prep_dma_memset():

drivers/dma/tegra186-gpc-dma.c:tegra_dma_prep_dma_memset() {
    ...
    sg_req[0].wcount = ((len - 4) >> 2);
    ...
}

The same calculation exists in tegra_dma_prep_dma_memcpy() and
tegra_dma_prep_slave_sg().

If a client requests a 0-length DMA transfer, the length checks appear to pass.
The computation then underflows to 0x3FFFFFFF. Because the hardware
interprets this as N+1 words, could this trigger an unbounded 4GB DMA
transfer?


[Severity: High]
This isn't a bug introduced by this patch, but does get_transfer_param()
silently propagate an error code to the MMIO sequence register?

drivers/dma/tegra186-gpc-dma.c:get_transfer_param() {
    ...
    *mmio_seq = get_bus_width(tdc, tdc->dma_sconfig.dst_addr_width);
    *burst_size = tdc->dma_sconfig.dst_maxburst;
    *slave_bw = tdc->dma_sconfig.dst_addr_width;
    *csr = TEGRA_GPCDMA_CSR_DMA_MEM2IO_FC;
    return 0;
}

If a client configures an unsupported dst_addr_width, get_bus_width() returns
-EINVAL. This negative error code is assigned directly to *mmio_seq, and the
function still returns 0 for success.

Could this signed error be bitwise-ORed with other flags and written directly
to the hardware register, corrupting the configuration bitfields?

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