[PATCH] dmaengine: idxd: fix double free in idxd_alloc() error path

[PATCH] dmaengine: idxd: fix double free in idxd_alloc() error path

[Guangshuo Li]

When dev_set_name() fails after device_initialize(), idxd_alloc()
calls put_device(conf_dev).

For these devices, conf_dev->type is set from idxd->data->dev_type,
which resolves to dsa_device_type or iax_device_type, and both use
idxd_conf_device_release() as their release callback.

That release callback frees idxd, idxd->opcap_bmap, and releases
idxd->id, but the current error path then frees those resources again
directly, causing a double free.

Keep the cleanup in idxd_conf_device_release() after put_device() and
avoid freeing idxd-managed resources again in idxd_alloc().

Fixes: 46a5cca76c76 ("dmaengine: idxd: fix memory leak in error handling path of idxd_alloc")
Cc: stable@vger.kernel.org
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
 drivers/dma/idxd/init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index 4eff74182225..94ce52565e7a 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -635,7 +635,7 @@ static struct idxd_device *idxd_alloc(struct pci_dev *pdev, struct idxd_driver_d
 
 err_name:
 	put_device(conf_dev);
-	bitmap_free(idxd->opcap_bmap);
+	return NULL;
 err_opcap:
 	ida_free(&idxd_ida, idxd->id);
 err_ida:
-- 
2.43.0

Re: [PATCH] dmaengine: idxd: fix double free in idxd_alloc() error path

[Vinicius Costa Gomes]

Hi,

Guangshuo Li <lgs201920130244@gmail.com> writes:

> When dev_set_name() fails after device_initialize(), idxd_alloc()
> calls put_device(conf_dev).
>
> For these devices, conf_dev->type is set from idxd->data->dev_type,
> which resolves to dsa_device_type or iax_device_type, and both use
> idxd_conf_device_release() as their release callback.
>
> That release callback frees idxd, idxd->opcap_bmap, and releases
> idxd->id, but the current error path then frees those resources again
> directly, causing a double free.
>
> Keep the cleanup in idxd_conf_device_release() after put_device() and
> avoid freeing idxd-managed resources again in idxd_alloc().
>
> Fixes: 46a5cca76c76 ("dmaengine: idxd: fix memory leak in error handling path of idxd_alloc")
> Cc: stable@vger.kernel.org
> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>

My preference is for the maintainer making the pull request to decide if
something should be sent to stable or not.

I was trying some AI review bot, I hope you don't mind, and got these
comments, went through them and they seemed good (including that these
patches should be sent as a series, that there are some more work to do
while you are cleaning the error paths), including it verbatim here:

This patch removes bitmap_free(idxd->opcap_bmap) after put_device()
in idxd_alloc()'s err_name path and adds a return NULL to prevent
falling through to the err_opcap and err_ida labels, avoiding
double-frees of opcap_bmap, ida, and idxd itself.

> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
> index 4eff74182225..94ce52565e7a 100644
> --- a/drivers/dma/idxd/init.c
> +++ b/drivers/dma/idxd/init.c
> @@ -635,7 +635,7 @@ static struct idxd_device *idxd_alloc(struct pci_dev *pdev, struct idxd_driver_d
>
>  err_name:
>  	put_device(conf_dev);
> -	bitmap_free(idxd->opcap_bmap);
> +	return NULL;
>  err_opcap:
>  	ida_free(&idxd_ida, idxd->id);
>  err_ida:

The double-free analysis is correct, but does the put_device() above
actually work here?

put_device(conf_dev) drops the refcount from 1 to 0 (no device_add()
was called, so nobody else holds a reference) and triggers the release
callback idxd_conf_device_release(), which does:

  idxd_conf_device_release() {
      destroy_workqueue(idxd->wq);
      ...
  }

At this point in idxd_alloc(), idxd->wq is still NULL -- the
workqueue is created much later in idxd_setup_internals():

  idxd_setup_internals() {
      ...
      idxd->wq = create_workqueue(dev_name(dev));
      ...
  }

destroy_workqueue() does not handle a NULL argument -- it immediately
dereferences the pointer:

  destroy_workqueue(wq) {
      workqueue_sysfs_unregister(wq);
      mutex_lock(&wq->mutex);          <-- NULL dereference
      ...
  }

So put_device() here will oops before the double-free is even
reached. This is a pre-existing issue (the old code has the same
put_device call), but relying on idxd_conf_device_release() as the
cleanup path for a partially-initialized idxd_device doesn't work.

Would it make sense to skip put_device() and instead free only
what was allocated, similar to the err_opcap and err_ida labels?

Two more things worth noting about this series:

Patch 3 (idxd_setup_engines) includes hunks that remove blank lines
from idxd_setup_groups() -- lines that only exist after Patch 2 is
applied. These four patches should probably be sent as a numbered
series with an explicit ordering rather than as independent patches.

The same put_device()-then-kfree() pattern also exists in
idxd_clean_wqs(), idxd_clean_engines(), idxd_clean_groups(), and
idxd_free(), which are not addressed by this series. It might be
worth fixing all of them together.


Cheers,
-- 
Vinicius

Re: [PATCH] dmaengine: idxd: fix double free in idxd_alloc() error path

[Guangshuo Li]

Hi Vinicius,

Thanks for reviewing  — the feedback is helpful.

I'm working on top of v6.19-rc8-214-ge7aa57247700.

Regarding the concern about put_device(conf_dev) triggering
idxd_conf_device_release() and hitting a NULL idxd->wq in
destroy_workqueue():

idxd_conf_device_release() does not call destroy_workqueue(). That
call lives in idxd_cleanup_internals(), which is a separate code path.
The actual release callback is:

static void idxd_conf_device_release(struct device *dev)
{
    struct idxd_device *idxd = confdev_to_idxd(dev);

    kfree(idxd->groups);
    bitmap_free(idxd->wq_enable_map);
    kfree(idxd->wqs);
    kfree(idxd->engines);
    kfree(idxd->evl);
    kmem_cache_destroy(idxd->evl_cache);
    ida_free(&idxd_ida, idxd->id);
    bitmap_free(idxd->opcap_bmap);
    kfree(idxd);
}

At the err_name point in idxd_alloc(), idxd was allocated with
kzalloc_node(), so all uninitialized fields are zero/NULL. Every
function in the release callback handles NULL safely:

kfree(NULL) — safe
bitmap_free(NULL) — safe (wraps kfree)
kmem_cache_destroy(NULL) — safe (explicit NULL check at entry)
ida_free(&idxd_ida, idxd->id) — id is already allocated at this point
bitmap_free(idxd->opcap_bmap) — already allocated at this point
So relying on put_device() → idxd_conf_device_release() to clean up is
correct for this error path.

Regarding the other points:

I agree the patches should be sent as a numbered series.
For the put_device()-then-kfree() double-free pattern in
idxd_clean_wqs(), idxd_clean_engines(), idxd_clean_groups(), and
idxd_free(), I'll address those in the same series.
Will send a v2 series shortly.

Thanks,
Guangshuo

Re: [PATCH] dmaengine: idxd: fix double free in idxd_alloc() error path

[Vinicius Costa Gomes]

Guangshuo Li <lgs201920130244@gmail.com> writes:

> Hi Vinicius,
>
> Thanks for reviewing  — the feedback is helpful.
>
> I'm working on top of v6.19-rc8-214-ge7aa57247700.
>
> Regarding the concern about put_device(conf_dev) triggering
> idxd_conf_device_release() and hitting a NULL idxd->wq in
> destroy_workqueue():
>
> idxd_conf_device_release() does not call destroy_workqueue(). That
> call lives in idxd_cleanup_internals(), which is a separate code path.
> The actual release callback is:
>

Current master includes that code:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/dma/idxd/sysfs.c#n1839

That modification was part of fix series that I proposed and was applied
on time for v7.0. It seems that I didn't do a good enough job of going
through the error paths.


Cheers,
-- 
Vinicius