Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] drm/amdxdna: long-term mm_users reference leak in UBUF BO --
commit 8ad12f14d7bf542056a227118a3d6363ee9a9c90 Author: Lizhi Hou <[email protected]> accel/amdxdna: Drop dma-buf wrapping for ubuf This commit makes ubuf a DRM GEM private object and tracks the user VA with HMM instead of a long-term pin. It creates the BO directly from the VA table without going through dma-buf. > diff --git a/drivers/accel/amdxdna/amdxdna_ubuf.c > b/drivers/accel/amdxdna/amdxdna_ubuf.c > index 0e0cd69cd1fbf..cff5d319ad605 100644 > --- a/drivers/accel/amdxdna/amdxdna_ubuf.c > +++ b/drivers/accel/amdxdna/amdxdna_ubuf.c > @@ -35,16 +32,13 @@ static void amdxdna_gem_ubuf_obj_free(struct > drm_gem_object *gobj) > { > + struct amdxdna_gem_obj *abo = to_xdna_obj(gobj); > + > + amdxdna_hmm_unreg_umaps(abo, true); > + cancel_work_sync(&abo->hmm_unreg_work); > + > + if (abo->mem.mm) > + mmput(abo->mem.mm); [Severity: High] Does this code leak the process's entire virtual address space if the BO outlives the creating process? A process calling the create BO ioctl to create a ubuf BO uses mmget_not_zero() in amdxdna_alloc_ubuf_bo() to store an mm_users reference on the creating process's mm. If the process then forks or passes the DRM file descriptor to another process and exits, the GEM object outlives the original process. The mm_users reference is held here until the BO is freed, preventing __mmput() from executing and tearing down the original address space (VMAs, page tables, anonymous memory). Could a malicious application repeatedly create processes, allocate ubuf BOs, pass the DRM fd to a daemon, and exit, rapidly exhausting system memory? > + drm_gem_object_release(gobj); > + amdxdna_gem_destroy_obj(abo); > } [ ... ] > @@ -91,31 +95,28 @@ struct amdxdna_gem_obj *amdxdna_alloc_ubuf_bo(struct > amdxdna_client *client, > + abo->type = AMDXDNA_BO_SHARE; > + abo->mem.uva = va_ent->vaddr; > + to_gobj(abo)->funcs = &amdxdna_gem_ubuf_obj_funcs; > + drm_gem_private_object_init(&xdna->ddev, to_gobj(abo), bufsize); > + > + if (!mmget_not_zero(client->mm)) { > + ret = -EFAULT; > + goto put_obj; > + } > + > + abo->mem.mm = client->mm; [Severity: High] Is it safe to establish this persistent mm_users reference? This ties the BO's lifetime to the process's full memory footprint, leading to the leak described above in amdxdna_gem_ubuf_obj_free(). > + > + ret = amdxdna_ubuf_hmm_register(abo, va_ent); > + if (ret) > + goto put_obj; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
