Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] amdxdna_gem_dmabuf_mmap() ignores the requested VMA page offset when mapping exported dma-bufs. - [High] Double `drm_gem_object_put()` in the `amdxdna_insert_pages()` error path causes a use-after-free. - [High] Asynchronous use-after-free of the `vma` pointer in `amdxdna_umap_release()`. -- commit 719267a0bdbc7550a107cb7df5bae4b3e649880b Author: Wentao Liang <[email protected]> accel/amdxdna: Fix use-after-free in amdxdna_gem_dmabuf_mmap() When vm_insert_pages() fails, the error path calls vma->vm_ops->close(vma) which internally calls drm_gem_vm_close() -> drm_gem_object_put(), releasing the gem object reference acquired at the start of the function. Cc: [email protected] > diff --git a/drivers/accel/amdxdna/amdxdna_gem.c > b/drivers/accel/amdxdna/amdxdna_gem.c > index 63976c3bcbe0f..91323dddad445 100644 > --- a/drivers/accel/amdxdna/amdxdna_gem.c > +++ b/drivers/accel/amdxdna/amdxdna_gem.c > @@ -527,6 +527,7 @@ static int amdxdna_gem_dmabuf_mmap(struct dma_buf > *dma_buf, struct vm_area_struc > > close_vma: > vma->vm_ops->close(vma); [Severity: High] This is a pre-existing issue, but does this function ignore the requested vma page offset when mapping exported dma-bufs? A few lines above in amdxdna_gem_dmabuf_mmap(), the base pages array is passed to vm_insert_pages() without being offset: ret = vm_insert_pages(vma, vma->vm_start, abo->base.pages, &num_pages); When an exported dma-buf is mapped by userspace, vma->vm_pgoff specifies the real offset within the buffer in pages. Because the base pages array is passed directly, if userspace requests a mapping with a non-zero offset, could it erroneously map the beginning of the buffer instead, corrupting the userspace memory view for partial or offset mappings? > + return ret; > put_obj: > drm_gem_object_put(gobj); > return ret; While reviewing this code, I also noticed a couple of other pre-existing issues in the amdxdna driver that might be worth looking into. [Severity: High] This is a pre-existing issue, but is there a double drm_gem_object_put() in the amdxdna_insert_pages() error path that causes a use-after-free? When vm_insert_pages() fails inside amdxdna_insert_pages(), the driver attempts to clean up by manually calling the close handler: XDNA_ERR(xdna, "Failed insert pages %d", ret); vma->vm_ops->close(vma); return ret; Because vma->vm_ops is assigned to &drm_gem_shmem_vm_ops, this triggers drm_gem_shmem_vm_close(), which unconditionally drops a gem object reference via drm_gem_vm_close(). The function then returns an error back up to drm_gem_mmap_obj(). However, drm_gem_mmap_obj() handles .mmap callback failures by putting the gem object reference itself on its error path. Does this result in the same gem object reference being put twice, leading to a refcount underflow and use-after-free? [Severity: High] This is a pre-existing issue, but could there be an asynchronous use-after-free of the vma pointer in amdxdna_umap_release()? amdxdna_hmm_register() caches the userspace vma pointer in mapp->vma. During memory unmapping, amdxdna_hmm_invalidate() receives an MMU_NOTIFY_UNMAP event and asynchronously queues mapp->hmm_unreg_work to a workqueue, but it does not block waiting for the work to complete. Consequently, the core mm proceeds to tear down and free the vma via vm_area_free(). When the asynchronous worker eventually executes amdxdna_umap_release(), it dereferences the stale vma pointer: if (is_import_bo(abo) && vma->vm_file && vma->vm_file->f_mapping) mapping_clear_unevictable(vma->vm_file->f_mapping); Since the vma has likely already been freed by the core mm, does this dereference trigger a use-after-free? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
