Re: [PATCH V2] accel/amdxdna: Fix potential NULL pointer dereference of abo->client

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Double put of GEM object on the error path of amdxdna_insert_pages()
- [High] Missing self-import check in amdxdna_gem_prime_import causes duplicate GEM objects
--

--- Patch [1]: [PATCH V2] accel/amdxdna: Fix potential NULL pointer dereference of abo->client ---
commit d74333f9f2549a77933710d6e3e66ba616b070a4
Author: Lizhi Hou <lizhi.hou@amd.com>

accel/amdxdna: Fix potential NULL pointer dereference of abo->client

This commit fixes a potential NULL pointer dereference when a BO handle is
closed but the underlying GEM object remains alive. It avoids accessing
abo->client in code paths executing after the handle closure.

> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index c562d3ae6f3f5..1adb7e57f50ce 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem in
amdxdna_insert_pages() while reviewing this file:

        ret = vm_insert_pages(vma, vma->vm_start, abo->base.pages,
                              &num_pages);
        if (ret) {
                XDNA_ERR(xdna, "Failed insert pages %d", ret);
                vma->vm_ops->close(vma);
                return ret;
        }

Does calling close() here prematurely drop the GEM object reference?

When userspace calls mmap() on an AMDXDNA BO, drm_gem_mmap_obj() acquires a
reference to the GEM object and calls the driver's .mmap callback, which
eventually calls amdxdna_insert_pages().

If vm_insert_pages() fails, the driver explicitly calls
vma->vm_ops->close(vma), which drops the GEM object reference. The error is
then returned back to drm_gem_mmap_obj(), which executes its own error path
and calls drm_gem_object_put() a second time.

Could this double-put cause an underflow of the object's reference count and
lead to a use-after-free of the GEM object?


[Severity: High]
This is also a pre-existing issue, but does amdxdna_gem_prime_import() miss
a self-import check?

struct drm_gem_object *
amdxdna_gem_prime_import(struct drm_device *dev, struct dma_buf *dma_buf)
{
        struct dma_buf_attachment *attach;
        struct amdxdna_gem_obj *abo;
        struct drm_gem_object *gobj;
        struct sg_table *sgt;
        int ret;

        get_dma_buf(dma_buf);

        attach = dma_buf_attach(dma_buf, dev->dev);

Since the DRM core defers to the driver's .gem_prime_import callback, and
this function unconditionally creates a new amdxdna_gem_obj without checking
if the DMA-BUF was originally exported by the same device, could this create
duplicate GEM objects for the same memory?

It looks like unprivileged userspace could export a BO via PRIME to a file
descriptor and import it back into the same device, which might break handle
de-duplication and cause state tracking inconsistencies.

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