Re: [PATCH] dmaengine: idxd: Fix use-after-free in idxd_remove

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Pre-existing Use-After-Free and Double Free in the error path of `idxd_register_devices()` and `idxd_pci_probe_alloc()`.
- [High] Pre-existing double free and UAF of the IDXD device and its resources in `idxd_free()`.
- [High] Flawed `while (j--)` loop condition in device registration cleanup paths causes an out-of-bounds array read on the first iteration failure, and skips unregistering elements on subsequent iteration failures.
- [Medium] Pre-existing memory leak of the `saved_engines` array in the error paths of `idxd_device_config_save`.
--

commit cf8581894d15c4c8e08a44d0ae7b7e0a1a4eb54c
Author: Ziyou Wang (Lenovo) <ziyou.dev@gmail.com>

dmaengine: idxd: Fix use-after-free in idxd_remove

This commit fixes a use-after-free bug in the idxd driver unbind path
by taking reference counts on the wq, engine, and group devices before
unregistering them. It adds idxd_put_devices() to release these references
after all cleanup operations are complete.

> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
> index 4b827a3297564..212efbfebacce 100644
> --- a/drivers/dma/idxd/init.c
> +++ b/drivers/dma/idxd/init.c

[ ... ]

[Severity: Medium]
This is a pre-existing issue, but does idxd_device_config_save() leak the
saved_engines array on error?

Looking at idxd_device_config_save() in drivers/dma/idxd/init.c:

	struct idxd_engine **saved_engines =
			kcalloc_node(idxd->max_engines,
				     sizeof(struct idxd_engine *),
				     GFP_KERNEL, dev_to_node(dev));

The saved_engines array is allocated without the __free(kfree) cleanup
attribute. If a memory allocation fails later in the function (for example,
when the device undergoes a reset and memory allocation fails), it jumps to
an error path that returns -ENOMEM.

The idxd_free_saved() function frees the individual structures via
idxd_free_saved_configs(), but assumes the arrays themselves are freed by the
__free cleanup mechanism. Since saved_engines lacks this attribute, is its
memory permanently leaked here?


[Severity: High]
This is a pre-existing issue, but can idxd_free() trigger a double-free
and use-after-free?

Looking at idxd_free() in drivers/dma/idxd/init.c:

static void idxd_free(struct idxd_device *idxd)
{
	if (!idxd)
		return;

	put_device(idxd_confdev(idxd));
	bitmap_free(idxd->opcap_bmap);
	ida_free(&idxd_ida, idxd->id);
	kfree(idxd);
}

When idxd_free() is called on early probe error paths, it first calls
put_device(idxd_confdev(idxd)). This drops the device refcount to 0 and
triggers the idxd_conf_device_release() callback, which explicitly frees
internal structures, calls bitmap_free(idxd->opcap_bmap),
ida_free(&idxd_ida, idxd->id), and then kfree(idxd).

After put_device() returns, idxd_free() immediately repeats these exact
same cleanup steps on the now-freed idxd structure. Does this result in a
direct use-after-free and double-free?

> diff --git a/drivers/dma/idxd/sysfs.c b/drivers/dma/idxd/sysfs.c
> index 6d251095c350f..eca96a4dcecd8 100644
> --- a/drivers/dma/idxd/sysfs.c
> +++ b/drivers/dma/idxd/sysfs.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but does idxd_register_devices() have a similar
use-after-free and double-free pattern in its error path?

Looking at idxd_register_devices() in drivers/dma/idxd/sysfs.c:

 err_group:
	for (i = 0; i < idxd->max_engines; i++)
		device_unregister(engine_confdev(idxd->engines[i]));
 err_engine:
	for (i = 0; i < idxd->max_wqs; i++)
		device_unregister(wq_confdev(idxd->wqs[i]));
 err_wq:
	device_del(idxd_confdev(idxd));

If device_add() fails in idxd_register_wq_devices() or subsequent steps,
this cleanup path calls device_unregister() on previously added engines and
WQs, freeing them. It then calls device_del(idxd_confdev(idxd)), which
unbinds the driver. This invokes idxd_device_drv_remove():

drivers/dma/idxd/device.c:idxd_device_drv_remove() {
	...
	for (i = 0; i < idxd->max_wqs; i++) {
		struct idxd_wq *wq = idxd->wqs[i];
		struct device *wq_dev = wq_confdev(wq);

		if (wq->state == IDXD_WQ_DISABLED)
			continue;
	...
}

This loop blindly accesses the WQ states which have already been freed by
the sysfs registration error handler, causing a direct use-after-free.

Furthermore, after idxd_register_devices() returns an error,
idxd_pci_probe_alloc() in drivers/dma/idxd/init.c jumps to err_dev_register:

drivers/dma/idxd/init.c:idxd_pci_probe_alloc() {
	...
 err_dev_register:
	idxd_cleanup(idxd);
	...
}

Which eventually calls idxd_clean_wqs():

drivers/dma/idxd/init.c:idxd_clean_wqs() {
	...
	for (i = 0; i < idxd->max_wqs; i++) {
		conf_dev = wq_confdev(idxd->wqs[i]);
		put_device(conf_dev);
	}
	...
}

This blindly drops references on objects that were already freed in the
idxd_register_devices() rollback, triggering a double free.


[Severity: High]
This is a pre-existing issue, but do the registration error paths have an
out-of-bounds array read?

Looking at idxd_register_wq_devices() in drivers/dma/idxd/sysfs.c:

	while (j--) {
		wq = idxd->wqs[j];
		device_unregister(wq_confdev(wq));
	}

In idxd_register_wq_devices(), idxd_register_engine_devices(), and
idxd_register_group_devices(), the cleanup paths use j = i - 1 followed by
while (j--).

If device_add() fails on the very first element (i = 0), j becomes -1. The
while (j--) condition evaluates the current value -1 as true (non-zero) and
decrements j to -2. The loop body then accesses index -2 (for example,
idxd->wqs[-2]) and passes the out-of-bounds pointer to device_unregister().

Conversely, if it fails on the second element (i = 1), j becomes 0. The
loop evaluates 0 as false and never executes. Does this skip unregistering
and leak the successfully added element at index 0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911065547.161522-1-ziyou.dev@gmail.com?part=1