Re: [PATCH v2 1/2] dmaengine: idxd: Do not call destroy_workqueue with null idxd->wq

Re: [PATCH v2 1/2] dmaengine: idxd: Do not call destroy_workqueue with null idxd->wq

[sashiko-bot]

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

Pre-existing issues:
- [Critical] The driver violates Linux device model lifetime rules, causing widespread double-free and use-after-free bugs in error paths and cleanup routines.
- [Critical] Error cleanup loops for sysfs device registration incorrectly use `while (j--)`, causing an out-of-bounds access or a resource leak depending on the failure iteration.
- [High] The patch adds a NULL check for `idxd->wq` in `idxd_conf_device_release()` but fails to handle dangling pointers, leaving a double-destroy vulnerability.
--

commit b2f5425f47723d096641d78d72f8c45caffb8a7a
Author: Steve Wahl <steve.wahl@hpe.com>
Subject: dmaengine: idxd: Do not call destroy_workqueue with null idxd->wq

This patch prevents calling destroy_workqueue() with a null pointer in
idxd_conf_device_release() by checking that idxd->wq is allocated. This
avoids a crash during error paths where the allocation has not yet occurred.

> diff --git a/drivers/dma/idxd/sysfs.c b/drivers/dma/idxd/sysfs.c
> index 6d251095c350f..d5ffc641c8567 100644
> --- a/drivers/dma/idxd/sysfs.c
> +++ b/drivers/dma/idxd/sysfs.c
> @@ -1836,7 +1836,8 @@ static void idxd_conf_device_release(struct device *dev)
>  {
>  	struct idxd_device *idxd = confdev_to_idxd(dev);
>  
> -	destroy_workqueue(idxd->wq);
> +	if (idxd->wq)
> +		destroy_workqueue(idxd->wq);

[Severity: High]
This is a pre-existing issue, but is there a double-destroy regression
here if idxd_cleanup_internals() was called?

If idxd_register_devices() fails, the cleanup code calls
idxd_cleanup_internals() which explicitly destroys idxd->wq but does not set
it to null. When the error propagates to idxd_free() and put_device() is
called, this release callback will be triggered. Because idxd->wq retains its
stale pointer, this check will pass and we will call destroy_workqueue() a
second time.

Should idxd_cleanup_internals() set idxd->wq to null after destroying it?

>  	kfree(idxd->groups);
>  	bitmap_free(idxd->wq_enable_map);
>  	kfree(idxd->wqs);

[Severity: Critical]
This isn't a bug introduced by this patch, but are there use-after-free and
double-free regressions due to device model lifetime violations?

When put_device() drops the refcount to 0, it automatically triggers this
release callback which correctly frees the parent structures and nested
allocations like idxd->wqs and idxd->groups.

However, in init.c routines like idxd_clean_wqs(), the code explicitly calls
kfree(wq) and bitmap_free(wq->opcap_bmap) before or after calling
put_device(). Does this cause a double-free when the release callbacks also
free these resources?

Additionally, when arrays like idxd->wqs are freed in those cleanup functions
but not set to null, won't this release callback free them a second time when
called via idxd_free()?

[Severity: Critical]
This is a pre-existing issue, but does the error cleanup loop for sysfs
device registration in idxd_register_engine_devices() and similar functions
cause an out-of-bounds access or leak?

In those functions, the error paths do this:

    j = i - 1;
    while (j--) {
        device_unregister(...);
    }

If device_add() fails on the very first element (i = 0), j initializes to -1.
Because -1 evaluates to true, the loop executes and decrements j to -2,
which could trigger an out-of-bounds array read (e.g., idxd->wqs[-2]) and a
crash.

If device_add() fails on the next iteration (i = 1), j initializes to 0.
The condition while (0--) evaluates to false, terminating immediately without
unregistering the successfully added element at index 0, leaking the device.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522203414.336549-1-steve.wahl@hpe.com?part=1