Re: [PATCH V2 3/3] accel/amdxdna: Fix amdxdna_client race in amdxdna_show_fdinfo()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The patch addresses a Use-After-Free in `amdxdna_show_fdinfo()`, but identical UAF vulnerabilities exist across most ioctls and GEM destruction paths due to broken client lifecycle management.
- [Critical] Premature destruction of `notifier_wq` in `amdxdna_remove()` causes a Use-After-Free when GEM objects are subsequently freed.
- [High] `amdxdna_sva_init()` leaves `client->sva` as a dangling pointer if `iommu_sva_get_pasid()` fails, leading to a double `iommu_sva_unbind_device()` on client cleanup.
--

commit 225c80eb0fc36a5511f44ce636794ce0b887dbc7
Author: Lizhi Hou <lizhi.hou@amd.com>

accel/amdxdna: Fix amdxdna_client race in amdxdna_show_fdinfo()

This patch guards the show_fdinfo path with drm_dev_enter() and
drm_dev_exit() to prevent use-after-free of the amdxdna_client structure
after the device has been unplugged.

> diff --git a/drivers/accel/amdxdna/amdxdna_pci_drv.c b/drivers/accel/amdxdna/amdxdna_pci_drv.c
> index c18ddad9f6459..e6b541db38981 100644
> --- a/drivers/accel/amdxdna/amdxdna_pci_drv.c
> +++ b/drivers/accel/amdxdna/amdxdna_pci_drv.c
> @@ -296,6 +296,10 @@ static void amdxdna_show_fdinfo(struct drm_printer *p, struct drm_file *filp)
>  	struct amdxdna_client *client = filp->driver_priv;
>  	size_t heap_usage, external_usage, internal_usage;
>  	char *drv_name = filp->minor->dev->driver->name;
> +	int idx;
> +
> +	if (!drm_dev_enter(filp->minor->dev, &idx))
> +		return;

[Severity: Critical]
This is a pre-existing issue, but while this patch addresses the UAF in
amdxdna_show_fdinfo(), do the same vulnerabilities exist across other ioctls
and GEM destruction paths due to broken client lifecycle management?

For example, amdxdna_remove() forcefully frees each amdxdna_client struct
by calling amdxdna_client_cleanup() while file descriptors might still be
open:

drivers/accel/amdxdna/amdxdna_pci_drv.c:amdxdna_remove() {
    ...
    client = list_first_entry_or_null(&xdna->client_list, ...);
    while (client) {
        amdxdna_client_cleanup(client);
        ...
    }
}

Meanwhile, IOCTL handlers like amdxdna_drm_submit_cmd_ioctl(),
amdxdna_drm_config_hwctx_ioctl(), and amdxdna_drm_wait_cmd_ioctl() omit
drm_dev_enter(), extracting the client pointer directly from
filp->driver_priv:

drivers/accel/amdxdna/amdxdna_ctx.c:amdxdna_drm_submit_cmd_ioctl() {
    struct amdxdna_client *client = filp->driver_priv;
    ...
    return amdxdna_drm_submit_execbuf(client, args);
}

If amdxdna_remove() runs concurrently, will these IOCTLs access the freed
client structure, causing a Use-After-Free? Similarly, when the file
descriptor is eventually closed post-unplug, will GEM destruction paths
like amdxdna_gem_obj_close() and amdxdna_gem_dev_obj_free() dereference the
already-freed abo->client pointer?


>  
>  	mutex_lock(&client->mm_lock);
>  

[ ... ]

[Severity: High]
This is a pre-existing issue, but does amdxdna_sva_init() leave client->sva
as a dangling pointer if iommu_sva_get_pasid() fails?

In amdxdna_sva_init(), if iommu_sva_bind_device() succeeds but getting the
pasid fails, the device is unbound but client->sva is not set to NULL:

drivers/accel/amdxdna/amdxdna_pci_drv.c:amdxdna_sva_init() {
    ...
    client->pasid = iommu_sva_get_pasid(client->sva);
    if (client->pasid == IOMMU_PASID_INVALID) {
        iommu_sva_unbind_device(client->sva);
        XDNA_ERR(xdna, "SVA get pasid failed");
        return -ENODEV;
    }
}

Then in amdxdna_drm_open(), if amdxdna_sva_init() fails but
amdxdna_use_carveout(xdna) is true, the driver issues a warning and allows
the client initialization to succeed.

When the file descriptor is closed and amdxdna_client_cleanup() calls
amdxdna_sva_fini(), will this dangling pointer bypass the IS_ERR_OR_NULL()
check and cause a double iommu_sva_unbind_device()?

drivers/accel/amdxdna/amdxdna_pci_drv.c:amdxdna_sva_fini() {
    if (IS_ERR_OR_NULL(client->sva))
        return;

    iommu_sva_unbind_device(client->sva);
}

[Severity: Critical]
This is a pre-existing issue, but does the premature destruction of
notifier_wq in amdxdna_remove() cause a Use-After-Free when GEM objects
are subsequently freed?

amdxdna_remove() destroys the workqueue before GEM objects are completely
released:

drivers/accel/amdxdna/amdxdna_pci_drv.c:amdxdna_remove() {
    ...
    destroy_workqueue(xdna->notifier_wq);
    ...
    drm_dev_unplug(&xdna->ddev);
}

When a GEM object's refcount drops to zero later, amdxdna_gem_obj_free()
is invoked and unconditionally calls flush_workqueue() on the destroyed
workqueue:

drivers/accel/amdxdna/amdxdna_gem.c:amdxdna_gem_obj_free() {
    ...
    amdxdna_hmm_unregister(abo, NULL);
    flush_workqueue(xdna->notifier_wq);
}

Could the destruction of the workqueue be deferred to the DRM device release
callback instead of the PCI remove callback?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608161418.2748988-1-lizhi.hou@amd.com?part=3