Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [High] Double-put of GEM object on the error path of `amdxdna_insert_pages()` for native BOs. - [High] Unconditional dereference of `vma->vm_ops->close` on the error path for imported BOs in `amdxdna_insert_pages()`. - [High] Use-after-free of VMA pointer stored in `amdxdna_umap` struct. - [High] The dmabuf mmap implementation ignores the user-requested mapping offset (`vma->vm_pgoff`). -- commit 09558bc86ce19ddb4577a60f4ba619ba0c0f3e1c Author: Jhonraushan <[email protected]> accel/amdxdna: Fix double drm_gem_object_put() in dma-buf mmap error path This commit alters the error path of amdxdna_gem_dmabuf_mmap() to return directly after vm_ops->close(vma). This prevents falling through to a second drm_gem_object_put(), fixing a double-free use-after-free issue. > Return directly after vm_ops->close() instead of falling through to the > extra drm_gem_object_put(), matching the sibling amdxdna_insert_pages() > error path. [Severity: High] This isn't a bug introduced by this patch, but the commit message mentions matching the error path in amdxdna_insert_pages(). Does amdxdna_insert_pages() have a similar double-put issue? If vm_insert_pages() fails in amdxdna_insert_pages(), it calls vma->vm_ops->close(vma). For native BOs, drm_gem_shmem_vm_close() drops the GEM object reference. The function then returns an error code up the stack to drm_gem_mmap_obj(). Since the .mmap callback returns an error, drm_gem_mmap_obj() intercepts it: drm_gem_mmap_obj() { ... err_drm_gem_object_put: drm_gem_object_put(obj); return ret; } Will this result in the object reference being dropped twice on that failure path, leading to a use-after-free? [Severity: High] This is a pre-existing issue, but looking at the imported BOs error path in amdxdna_insert_pages(): amdxdna_insert_pages() { ... if (fault_ret & VM_FAULT_ERROR) { vma->vm_ops->close(vma); XDNA_ERR(xdna, "Fault in page failed"); return -EFAULT; } } Is it safe to unconditionally call vma->vm_ops->close() here? If dma_buf_mmap() sets up the VMA with an exporter's vm_ops that doesn't provide a .close callback, or if vma->vm_ops is NULL, could this cause a NULL pointer dereference? [Severity: High] This is another pre-existing issue, but I noticed a potential lifetime issue with the VMA pointer in the hmm tracking code. In amdxdna_hmm_register(), the driver stores the raw VMA pointer: amdxdna_hmm_register() { ... mapp->vma = vma; mapp->abo = abo; } Since the VMA's lifetime is tied to the process's mm and is destroyed by the core MM subsystem during munmap(), could the asynchronous work amdxdna_umap_release() dereference a freed pointer? amdxdna_umap_release() { ... if (is_import_bo(abo) && vma->vm_file && vma->vm_file->f_mapping) mapping_clear_unevictable(vma->vm_file->f_mapping); } If the MM subsystem frees the VMA struct before this workqueue executes, does this lead to a use-after-free crash? > diff --git a/drivers/accel/amdxdna/amdxdna_gem.c > b/drivers/accel/amdxdna/amdxdna_gem.c > --- a/drivers/accel/amdxdna/amdxdna_gem.c > +++ b/drivers/accel/amdxdna/amdxdna_gem.c [ ... ] > @@ -526,7 +526,12 @@ static int amdxdna_gem_dmabuf_mmap(struct dma_buf > *dma_buf, struct vm_area_struc [Severity: High] This is a pre-existing issue, but does this function ignore the user-requested page offset? Looking at the code just above this hunk: amdxdna_gem_dmabuf_mmap() { ... ret = vm_insert_pages(vma, vma->vm_start, abo->base.pages, &num_pages); if (ret) goto close_vma; } By unconditionally passing abo->base.pages to vm_insert_pages(), a user mapping the dmabuf at a non-zero offset (vma->vm_pgoff) will receive the beginning of the buffer rather than the requested offset. Should this code offset the page array using abo->base.pages + vma->vm_pgoff, or use vm_map_pages() instead? > return 0; > > close_vma: > + /* vm_ops->close() drops the reference taken by drm_gem_object_get() > + * above, so return directly instead of falling through to put_obj > + * and dropping it a second time. > + */ > vma->vm_ops->close(vma); > + return ret; > put_obj: > drm_gem_object_put(gobj); > return ret; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
