Re: [PATCH v3 19/34] dmaengine: xilinx-dma: use dmaengine BH instead of tasklets

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

Pre-existing issues:
- [Critical] Shared IRQ handler for MCDMA cross-schedules bottom halves for improperly masked removed channels, causing UAF.
- [High] Early IRQ request exposes uninitialized channel structures to premature interrupts, causing NULL pointer dereferences.
- [High] Unvalidated channel ID read from hardware causes an out-of-bounds array access.
- [High] Assuming all indexed channels in `xdev->chan` are fully populated causes a NULL pointer dereference for unprobed channels.
- [High] Missing bounds check on the `dma-channels` device tree property allows an out-of-bounds heap write.
--

--- Patch [19]: [PATCH v3 19/34] dmaengine: xilinx-dma: use dmaengine BH instead of tasklets ---
commit 2a691fa5fbfdf533a9f83a643b93d18aff3e2930
Author: Allen Pais <[email protected]>
Subject: dmaengine: xilinx-dma: use dmaengine BH instead of tasklets

This patch replaces per-channel tasklets with the shared dmaengine
bottom half (BH) helper mechanism. It transitions the tasklet cleanup
handler to run under the dmaengine's scheduling and teardown framework
while continuing to execute in softirq context.

> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index 404235c173538..b142c9c0583c7 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c

[ ... ]

> @@ -1898,7 +1897,7 @@ static irqreturn_t xilinx_mcdma_irq_handler(int irq, void *data)
>  		spin_unlock(&chan->lock);
>  	}
>  
> -	tasklet_hi_schedule(&chan->tasklet);
> +	dmaengine_schedule_bh(&chan->common);
>  	return IRQ_HANDLED;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but looking at how
xilinx_mcdma_irq_handler() computes the channel offset earlier in this
function:

    chan_sermask = dma_ctrl_read(chan, ser_offset);
    chan_id = ffs(chan_sermask);
    ...
    chan_offset = chan_offset + (chan_id - 1);
    chan = chan->xdev->chan[chan_offset];

Can a faulty hardware device or emulator asserting bits in the upper half of
chan_sermask cause an out-of-bounds access? The array xdev->chan is
statically sized to XILINX_MCDMA_MAX_CHANS_PER_DEVICE (32), but
chan_offset could potentially reach up to 47 for DEV_TO_MEM transfers.

[Severity: High]
This is also a pre-existing issue, but immediately after fetching the
channel pointer:

    chan = chan->xdev->chan[chan_offset];
    status = dma_ctrl_read(chan, XILINX_MCDMA_CHAN_SR_OFFSET(chan->tdest));

If the device tree did not define all channels or some failed to probe,
could the xdev->chan array contain NULL pointers? If the hardware asserts
interrupts for unpopulated channels, it seems this would cause a NULL
pointer dereference.

[ ... ]

> @@ -2745,7 +2744,7 @@ static void xilinx_dma_chan_remove(struct xilinx_dma_chan *chan)
>  	if (chan->irq > 0)
>  		free_irq(chan->irq, chan);
>  
> -	tasklet_kill(&chan->tasklet);
> +	dmaengine_kill_bh(&chan->common);
>  
>  	list_del(&chan->common.device_node);
>  }

[Severity: Critical]
This isn't a bug introduced by this patch, but looking at the teardown
sequence in xilinx_dma_chan_remove():

    dma_ctrl_clr(chan, XILINX_DMA_REG_DMACR,
                 XILINX_DMA_DMAXR_ALL_IRQ_MASK);

Does this write to the correct register for MCDMA channels? It appears to
write to XILINX_DMA_REG_DMACR instead of an MCDMA-specific register like
XILINX_MCDMA_CHAN_CR_OFFSET.

If the MCDMA hardware interrupts remain enabled, and the shared IRQ line
fires after this channel structure is freed, couldn't the shared
xilinx_mcdma_irq_handler() fetch this freed channel from the uncleared
xdev->chan array and call dmaengine_schedule_bh() on freed memory?

[ ... ]

> @@ -3075,8 +3074,7 @@ static int xilinx_dma_chan_probe(struct xilinx_dma_device *xdev,
>  			str_enabled_disabled(chan->has_sg));
>  	}
>  
> -	/* Initialize the tasklet */
> -	tasklet_setup(&chan->tasklet, xilinx_dma_do_tasklet);
> +	dmaengine_init_bh(&chan->common, xilinx_dma_do_tasklet);
>  
>  	/*
>  	 * Initialize the DMA channel and add it to the DMA engine channels

[Severity: High]
This isn't a bug introduced by this patch, but earlier in
xilinx_dma_chan_probe(), request_irq() is called before critical function
pointers like chan->start_transfer are initialized:

    err = request_irq(chan->irq, xdev->dma_config->irq_handler,
                      IRQF_SHARED, "xilinx-dma-controller", chan);

If a shared interrupt fires immediately (or if there is pending hardware
state), the handler xilinx_dma_irq_handler() will execute and might
unconditionally call chan->start_transfer(chan). Could this cause a NULL
pointer dereference since start_transfer is populated after request_irq()?

[Severity: High]
This is also a pre-existing issue, but further down in
xilinx_dma_chan_probe():

    xdev->chan[chan->id] = chan;

This relies on chan->id, which is incremented based on the loop in
xilinx_dma_child_probe():

    ret = of_property_read_u32(node, "dma-channels", &nr_channels);
    for (i = 0; i < nr_channels; i++) {
        ret = xilinx_dma_chan_probe(xdev, node);

Is there any bounds checking to ensure nr_channels or chan->id does not
exceed XILINX_MCDMA_MAX_CHANS_PER_DEVICE? Without it, an oversized device
tree property might cause an out-of-bounds heap write here.

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