Re: [PATCH v3 11/11] dmaengine: plx_dma: fix NULL pointer deref in plx_dma_isr()

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

Pre-existing issues:
- [High] Client callbacks are invoked with the driver's primary spinlock held, causing deadlocks if the callback initiates a new DMA transaction.
- [High] `plx_dma_prep_memcpy` returns with a spinlock held, violating API boundaries and causing deadlocks on multiple descriptor preparations.
- [High] Softirq deadlock due to failing to disable bottom halves before acquiring a spinlock in process context.
- [High] Missing read memory barrier after checking DMA descriptor status can lead to reading stale payload data.
--

--- Patch [11]: [PATCH v3 11/11] dmaengine: plx_dma: fix NULL pointer deref in plx_dma_isr() ---
commit 95d7de134f15bc6f73a67b043a21039f2fc9965a
Author: Logan Gunthorpe <[email protected]>

dmaengine: plx_dma: fix NULL pointer deref in plx_dma_isr()

plx_dma_create() registered the interrupt handler with request_irq()
before initializing plxdev->bar. If the device raised an interrupt in
that window, plx_dma_isr() would dereference the still-NULL bar.

Move the bar assignment ahead of request_irq() so everything the
handler can touch is initialized before it can run.

Link: https://lore.kernel.org/all/[email protected]

> diff --git a/drivers/dma/plx_dma.c b/drivers/dma/plx_dma.c
> index 84941a918b01f..409898e92c320 100644
> --- a/drivers/dma/plx_dma.c
> +++ b/drivers/dma/plx_dma.c
> @@ -504,17 +504,17 @@ static int plx_dma_create(struct pci_dev *pdev)
>  	if (!plxdev)
>  		return -ENOMEM;
>  
> -	rc = request_irq(pci_irq_vector(pdev, 0), plx_dma_isr, 0,
> -			 KBUILD_MODNAME, plxdev);
> -	if (rc)
> -		goto free_plx;
> -
>  	spin_lock_init(&plxdev->ring_lock);
>  	tasklet_setup(&plxdev->desc_task, plx_dma_desc_task);
>  
>  	RCU_INIT_POINTER(plxdev->pdev, pdev);
>  	plxdev->bar = pcim_iomap_table(pdev)[0];
>  
> +	rc = request_irq(pci_irq_vector(pdev, 0), plx_dma_isr, 0,
> +			 KBUILD_MODNAME, plxdev);
> +	if (rc)
> +		goto free_plx;
> +
>  	dma = &plxdev->dma_dev;
>  	INIT_LIST_HEAD(&dma->channels);
>  	dma_cap_set(DMA_MEMCPY, dma->cap_mask);

While reviewing this patch, I noticed a few pre-existing issues in this driver:

drivers/dma/plx_dma.c:plx_dma_process_desc() {
[ ... ]
	spin_lock(&plxdev->ring_lock);

[Severity: High]
This isn't a bug introduced by this patch, but can this cause a softirq
deadlock?

plx_dma_tx_status() is typically called from process context and invokes
plx_dma_process_desc(), which acquires this lock without disabling bottom
halves.

If a hardware interrupt occurs, plx_dma_isr() schedules a tasklet. Upon
returning from the interrupt, the tasklet executes in softirq context on the
same CPU and calls plx_dma_process_desc(), which attempts to acquire the
already-held non-recursive spinlock.

[ ... ]
		flags = le32_to_cpu(READ_ONCE(desc->hw->flags_and_size));

		if (flags & PLX_DESC_FLAG_VALID)
			break;

[Severity: High]
This isn't a bug introduced by this patch, but is a dma_rmb() needed here?

Without a read memory barrier after verifying the descriptor is marked valid
by the device, the CPU could speculatively reorder subsequent memory reads
before the status check completes. Could this lead to a client callback
reading stale payload data from the DMA destination buffer?

[ ... ]
		dmaengine_desc_get_callback_invoke(&desc->txd, &res);

[Severity: High]
This is a pre-existing issue, but could this cause a deadlock?

The client callback is invoked synchronously while plxdev->ring_lock is
held. If the client completion callback issues a new DMA transaction by
calling dmaengine_prep_*, it will call into plx_dma_prep_memcpy() which
attempts to acquire the same spinlock.

}


drivers/dma/plx_dma.c:plx_dma_prep_memcpy() {
[ ... ]
	spin_lock_bh(&plxdev->ring_lock);
[ ... ]
	/* return with the lock held, it will be released in tx_submit */

	return &plxdesc->txd;
}

[Severity: High]
This isn't a bug introduced by this patch, but does returning with the
spinlock held violate the dmaengine API contract?

If a client prepares two descriptors sequentially before submitting (e.g.,
calling prep() then prep() again), the second call will deadlock trying to
acquire the already-held lock. This also forces clients into an atomic
context between prep and submit.

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