Re: [PATCH v4 10/12] dmaengine: ioat: disable relaxed ordering before registering the device

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

Pre-existing issues:
- [High] Use-After-Free (UAF) in shared IRQ handler due to unsafe mixing of manual memory management and devres IRQ registration.
- [High] Global IDA resource leak in the core DMA channel registration error path.
- [Medium] Race condition exposing DMA channels to userspace before their sysfs attributes are fully created.
--

--- Patch [10]: [PATCH v4 10/12] dmaengine: ioat: disable relaxed ordering before registering the device ---
commit f2b93cc9001f8f226c2e33f07702973d69e902de
Author: Logan Gunthorpe <[email protected]>

dmaengine: ioat: disable relaxed ordering before registering the device

ioat3_dma_probe() disabled PCIe relaxed ordering after calling
dma_async_device_register(), so if an error occurs and the code jumps
to err_disable_interrupts, the function returns with a dangling registration.
This commit moves the capability read/write ahead of registration to fix
the issue.

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

> diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
> index 737496391109e..a57024c4b066e 100644
> --- a/drivers/dma/ioat/init.c
> +++ b/drivers/dma/ioat/init.c
> @@ -1170,15 +1170,6 @@ static int ioat3_dma_probe(struct ioatdma_device *ioat_dma, int dca)
>  		       ioat_chan->reg_base + IOAT_DCACTRL_OFFSET);
>  	}
>  
> -	err = dma_async_device_register(&ioat_dma->dma_dev);
> -	if (err)
> -		goto err_disable_interrupts;
> -
> -	ioat_kobject_add(ioat_dma, &ioat_ktype);
> -
> -	if (dca)
> -		ioat_dma->dca = ioat_dca_init(pdev, ioat_dma->reg_base);
> -
>  	/* disable relaxed ordering */
>  	err = pcie_capability_read_word(pdev, PCI_EXP_DEVCTL, &val16);
>  	if (err) {
> @@ -1194,6 +1185,15 @@ static int ioat3_dma_probe(struct ioatdma_device *ioat_dma, int dca)
>  		goto err_disable_interrupts;
>  	}
>  
> +	err = dma_async_device_register(&ioat_dma->dma_dev);

[Severity: High]
This is a pre-existing issue, but does dma_async_device_register() leak the
global dma_ida resource when it fails?

If dma_async_device_register() fails after allocating a global ID and
populating the channel list, the error path in drivers/dma/dmaengine.c handles
the cleanup:

	if (!device->chancnt) {
		ida_free(&dma_ida, device->dev_id);
		return rc;
	}

Because it skips ida_free() when chancnt is greater than zero, does this
permanently leak an ID from the global dma_ida pool every time registration
fails with populated channels?

> +	if (err)
> +		goto err_disable_interrupts;

[Severity: High]
This isn't a bug introduced by this patch, but is there a use-after-free risk
on this error path?

If ioat3_dma_probe() fails and the code takes the err_disable_interrupts
path, ioat_pci_probe() will manually free the primary device data structure.
However, because ioat_dma_setup_interrupts() registers a shared interrupt
using devm_request_irq(), the interrupt cleanup is deferred until after the
probe function completely returns.

Could a shared interrupt firing during this brief window invoke the handler,
causing ioat_dma_do_interrupt() to blindly dereference the already freed data
pointer?

> +
> +	ioat_kobject_add(ioat_dma, &ioat_ktype);

[Severity: Medium]
This is a pre-existing issue, but does this sequence expose DMA channels to
userspace before their sysfs attributes are fully created?

Because dma_async_device_register() completes core registration and sends
add uevents before ioat_kobject_add() appends the specific quickdata
sysfs attributes, can a userspace process reacting to the uevent attempt to
read device properties in the window before they are created?

> +
> +	if (dca)
> +		ioat_dma->dca = ioat_dca_init(pdev, ioat_dma->reg_base);
> +
>  	if (ioat_dma->cap & IOAT_CAP_DPS)
>  		writeb(ioat_pending_level + 1,
>  		       ioat_dma->reg_base + IOAT_PREFETCH_LIMIT_OFFSET);

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