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 <[email protected]> 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/[email protected]?part=1
