Re: [PATCH v1 4/6] 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] IRQ handler Use-After-Free in probe error path due to mixing devres IRQ registration with manual kfree.
- [High] Use-After-Free on `ioatdma_chan` due to missing kobject `.release` function and unconditional kfree.
- [High] Legacy MSI/MSI-X vectors leak on error paths and interrupt fallback, causing device dysfunction and resource exhaustion.
--

--- Patch [4]: [PATCH v1 4/6] dmaengine: ioat: disable relaxed ordering before registering the device ---
commit eb15a8e1f4e218e8537f96abc3be35c46bf67999
Author: Logan Gunthorpe <[email protected]>

dmaengine: ioat: disable relaxed ordering before registering the device

This commit fixes a dangling device registration in the error path of
ioat3_dma_probe(). By moving the relaxed ordering disablement before
dma_async_device_register(), it ensures the device is safely registered
only when subsequent initialization steps cannot fail.

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

> 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

[ ... ]

> @@ -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);
> +	if (err)
> +		goto err_disable_interrupts;
> +

[Severity: High]
This isn't a bug introduced by this patch, but doesn't this error path lead to
a use-after-free?

If dma_async_device_register() fails here and we jump to
err_disable_interrupts, the probe fails and ioat_pci_probe() will manually
call kfree() on the device and channel structures before returning:

drivers/dma/ioat/init.c:ioat_pci_probe() {
    ...
    for (i = 0; i < IOAT_MAX_CHANS; i++)
        kfree(device->idx[i]);
    kfree(device);
    ...
}

However, because devm_request_irq() was used earlier to register the interrupt
handler, devres will only unregister the IRQ after the probe function fully
returns. If a shared interrupt fires in this window, ioat_dma_do_interrupt()
will dereference the freed instance pointer causing a use-after-free.

[Severity: High]
This is a pre-existing issue, but does the error handling properly clean up
MSI/MSI-X vectors?

If we jump to err_disable_interrupts and fail probe, or during normal unbind,
the driver masks hardware interrupts but fails to call pci_disable_msi() or
pci_disable_msix(). For example in ioat_dma_remove():

drivers/dma/ioat/init.c:ioat_dma_remove() {
    ...
    ioat_disable_interrupts(ioat_dma);
    ...
}

This permanently leaks the allocated IRQ vectors. Also, if devm_request_irq()
fails during MSI-X setup, the driver jumps to the msi fallback label without
calling pci_disable_msix(). This breaks INTx generation since the device will
not assert INTx while MSI-X is active.

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

[Severity: High]
This is a pre-existing issue, but is it safe to embed kobjects here without a
release function?

When ioat_kobject_add() adds the kobject with ioat_ktype:

drivers/dma/ioat/sysfs.c:ioat_ktype {
    const struct kobj_type ioat_ktype = {
        .sysfs_ops = &ioat_sysfs_ops,
        .default_groups = ioat_groups,
    };
}

It is missing the required .release function. When the driver is unbound,
release_ioatdma() unconditionally frees the channel structures:

drivers/dma/ioat/init.c:release_ioatdma() {
    ...
    for (i = 0; i < IOAT_MAX_CHANS; i++)
        kfree(d->idx[i]);
    ...
}

If unprivileged userspace holds a sysfs file open while the device is unbound,
the kobject refcount is still elevated. When userspace later closes the file,
kobject_put() will attempt to drop the final reference and access the already
freed memory.

> +	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=4