Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [Critical] The patch fixes a NULL pointer dereference in `amdxdna_gem_vmap()` caused by `abo->client` being prematurely set to NULL, but misses identical vulnerabilities in other asynchronous and internal execution paths. - [High] Use-after-free of `amdxdna_client` when a GEM object is shared between processes via PRIME export/import. - [High] IOVA and IOMMU pagetable leak when a GEM object handle is closed and re-opened via PRIME. - [Medium] Data Race / Missing Memory Barriers in `amdxdna_gem_vmap` -- --- Patch [1]: [PATCH V1] accel/amdxdna: Fix potential NULL pointer access in amdxdna_gem_vmap() --- commit 6ad0ad0bac54f532a4aacd997b6204c5f05a71e3 Author: Lizhi Hou <[email protected]> Date: Tue Jul 7 17:23:16 2026 +0000 accel/amdxdna: Fix potential NULL pointer access in amdxdna_gem_vmap() If the heap BO was never vmapped before its handle was closed and the user then vmaps the dev BO (which triggers amdxdna_gem_vmap(heap_bo)), and drm_gem_vmap fails — NULL pointer dereference at abo->client->xdna. Fix it by get xdna pointer through to_xdna_dev(to_gobj(abo)->dev). > diff --git a/drivers/accel/amdxdna/amdxdna_gem.c > b/drivers/accel/amdxdna/amdxdna_gem.c > index 1f190b319bb6e..e7566cbe3da76 100644 > --- a/drivers/accel/amdxdna/amdxdna_gem.c > +++ b/drivers/accel/amdxdna/amdxdna_gem.c > @@ -198,6 +198,7 @@ amdxdna_gem_destroy_obj(struct amdxdna_gem_obj *abo) > */ > void *amdxdna_gem_vmap(struct amdxdna_gem_obj *abo) > { > + struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev); > struct iosys_map map = IOSYS_MAP_INIT_VADDR(NULL); > int ret; > > @@ -210,7 +211,7 @@ void *amdxdna_gem_vmap(struct amdxdna_gem_obj *abo) [Severity: Medium] This isn't a bug introduced by this patch, but does the double-checked locking pattern in amdxdna_gem_vmap() need memory barriers? The code leading up to this block reads abo->mem.kva locklessly: if (abo->mem.kva) return abo->mem.kva; /* The first call to get the kva, taking slow path. */ guard(mutex)(&abo->lock); Without smp_load_acquire() on the read and smp_store_release() on the write (when assigning abo->mem.kva = map.vaddr), could the compiler or CPU reorder instructions such that a concurrent reader sees a non-NULL pointer before the effects of drm_gem_vmap() are fully visible? > if (!abo->mem.kva) { > ret = drm_gem_vmap(to_gobj(abo), &map); > if (ret) > - XDNA_ERR(abo->client->xdna, "Vmap bo failed, ret %d", > ret); > + XDNA_ERR(xdna, "Vmap bo failed, ret %d", ret); [Severity: Critical] This isn't a bug introduced by this patch, but while fixing the NULL pointer dereference here, are there other identical vulnerabilities in asynchronous and internal execution paths? When userspace closes a GEM handle, amdxdna_gem_obj_close() drops open_ref and sets abo->client = NULL. However, the GEM object can remain alive due to internal kernel references. For example, in drivers/accel/amdxdna/aie2_message.c:aie2_init_exec_req(): struct amdxdna_dev *xdna = cmd_abo->client->xdna; If a user submits a job using a command BO and immediately closes its handle, the scheduler job holds a GEM reference keeping the object alive, but cmd_abo->client becomes NULL. When the scheduler thread executes aie2_init_exec_req(), will this dereference NULL? Similarly, in drivers/accel/amdxdna/amdxdna_gem.h:amdxdna_obj_dma_addr(): return amdxdna_pasid_on(abo->client) ? amdxdna_gem_uva(abo) : abo->mem.dma_addr; If the user allocates a new DEV BO, and amdxdna_init_dev_bo() invokes amdxdna_obj_dma_addr(heap), will it evaluate amdxdna_pasid_on(heap->client) which evaluates NULL->pasid? [Severity: High] This is a pre-existing issue, but how does the driver handle the abo->client pointer when a GEM object is shared between processes via PRIME export/import? Looking at drivers/accel/amdxdna/amdxdna_gem.c:amdxdna_gem_obj_open(): if (abo->open_ref > 1) return 0; If Process A creates a BO and exports it to Process B, amdxdna_gem_obj_open() is called for Process B. Since open_ref > 0, it simply increments open_ref and returns, leaving abo->client pointing to Process A's client structure. If Process A subsequently closes its handle and exits, its amdxdna_client structure is kfree'd. Later, when Process B closes its handle, open_ref reaches 0 and amdxdna_gem_obj_close() invokes amdxdna_gem_del_bo_usage(abo): struct amdxdna_client *client = abo->client; ... guard(mutex)(&client->mm_lock); Will this dereference the dangling client pointer and trigger a use-after-free? [Severity: High] This is a pre-existing issue, but does closing and re-opening a GEM object handle via PRIME leak IOVA and IOMMU pagetables? When a GEM object's handle is closed and open_ref drops to 0, amdxdna_gem_obj_close() does not unmap the DMA/IOVA mapping. If userspace retains the PRIME dmabuf FD and re-imports it, a new handle is created and amdxdna_gem_obj_open() is called: if (!amdxdna_pasid_on(abo->client)) { ret = amdxdna_dma_map_bo(xdna, abo); This unconditionally maps the BO when open_ref transitions from 0 to 1, regardless of existing mappings. Then in drivers/accel/amdxdna/amdxdna_iommu.c:amdxdna_dma_map_bo(): iova = amdxdna_iommu_alloc_iova(xdna, abo->mem.size, &dma_addr, ...); ... abo->mem.dma_addr = dma_addr; If IOVA is enabled, this allocates a new IOVA and overwrites abo->mem.dma_addr without checking if a mapping already exists. Are the previous IOVA and its IOMMU mappings permanently leaked? > else > abo->mem.kva = map.vaddr; > } -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
