Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] amdxdna: UBUF BOs trigger kernel WARN_ON and crash when opened by non-PASID client - [Medium] amdxdna: CREATE_BO ubuf rejects valid multi-VMA user buffers - [Medium] amdxdna: unprotected assignment to shared abo->mem.uva causes data race --
commit 1efe7ea3fe610d7e9d53d3b1fead24d8137d077d Author: Lizhi Hou <[email protected]> Subject: accel/amdxdna: Drop dma-buf wrapping for ubuf This commit drops dma-buf wrapping for ubuf, directly importing userspace memory as a DRM GEM private object and tracking the user VA with HMM. It removes the FOLL_LONGTERM pin and relies on an MMU interval notifier to fault the range in on command submission. > diff --git a/drivers/accel/amdxdna/amdxdna_gem.c > b/drivers/accel/amdxdna/amdxdna_gem.c > index f4832337ec31d..a3d9a54c65634 100644 > --- a/drivers/accel/amdxdna/amdxdna_gem.c > +++ b/drivers/accel/amdxdna/amdxdna_gem.c [ ... ] > @@ -335,12 +336,11 @@ static void amdxdna_hmm_unreg_work(struct work_struct > *work) > amdxdna_hmm_unreg_umaps(abo, false); > } > > -static int amdxdna_hmm_register(struct amdxdna_gem_obj *abo, > - struct vm_area_struct *vma) > +int amdxdna_hmm_register(struct amdxdna_gem_obj *abo, struct vm_area_struct > *vma, > + size_t offset, size_t len) > { > struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev); > - unsigned long len = vma->vm_end - vma->vm_start; > - unsigned long addr = vma->vm_start; > + unsigned long addr = vma->vm_start + offset; [Severity: Medium] If a process creates a BO, forks, and both parent and child processes concurrently mmap the inherited file descriptor, they can concurrently reach the early return path in this function where abo->mem.uva is set to addr. Does this unprotected assignment to abo->mem.uva cause a data race, potentially leading to corrupted addresses being returned via GET_BO_INFO? > struct amdxdna_umap *mapp; > unsigned long nr_pages; > int ret; [ ... ] > diff --git a/drivers/accel/amdxdna/amdxdna_ubuf.c > b/drivers/accel/amdxdna/amdxdna_ubuf.c > index 0e0cd69cd1fbf..5c291786d981b 100644 > --- a/drivers/accel/amdxdna/amdxdna_ubuf.c > +++ b/drivers/accel/amdxdna/amdxdna_ubuf.c [ ... ] > +static int amdxdna_ubuf_hmm_register(struct amdxdna_client *client, > + struct amdxdna_gem_obj *abo, > + struct amdxdna_drm_va_entry *va_ent) > { > - struct amdxdna_ubuf_priv *ubuf = attach->dmabuf->priv; > - struct sg_table *sg; > + struct vm_area_struct *vma; > int ret; > > - sg = kzalloc_obj(*sg); > - if (!sg) > - return ERR_PTR(-ENOMEM); > - > - ret = sg_alloc_table_from_pages(sg, ubuf->pages, ubuf->nr_pages, 0, > - ubuf->nr_pages << PAGE_SHIFT, > GFP_KERNEL); > - if (ret) > - goto err_free_sg; > + mmap_write_lock(client->mm); > + vma = find_vma(client->mm, va_ent->vaddr); > + if (!vma || vma->vm_start > va_ent->vaddr || > + vma->vm_end - va_ent->vaddr < va_ent->len) { > + ret = -EINVAL; > + goto unlock; > + } [Severity: Medium] If a user calls CREATE_BO with a ubuf that spans multiple VMAs (such as adjacent mmaps or split VMAs due to mprotect), find_vma() will return the first VMA. Does this check against vma->vm_end fail the entire request and incorrectly return -EINVAL for a valid multi-VMA userspace buffer? > - ret = dma_map_sgtable(attach->dev, sg, direction, 0); > - if (ret) > - goto err_free_table; > + ret = amdxdna_hmm_register(abo, vma, va_ent->vaddr - vma->vm_start, > va_ent->len); [ ... ] > +struct amdxdna_gem_obj *amdxdna_alloc_ubuf_bo(struct amdxdna_client *client, > + u32 num_entries, void __user > *va_entries) > { [ ... ] > + bufsize = va_ent->len; > + abo = amdxdna_gem_create_obj(&xdna->ddev, bufsize); > + if (IS_ERR(abo)) { > + ret = PTR_ERR(abo); > + goto free_ent; > } > > - for (i = 0; i < num_entries; i++) { > - npages = va_ent[i].len >> PAGE_SHIFT; > - > - ret = pin_user_pages_fast(va_ent[i].vaddr, npages, > - FOLL_WRITE | FOLL_LONGTERM, > - &ubuf->pages[start]); > - if (ret >= 0) { > - start += ret; > - if (ret != npages) { > - XDNA_ERR(xdna, "Partially pinned pages %d/%u", > ret, npages); > - ret = -ENOMEM; > - goto destroy_pages; > - } > - } else { > - XDNA_ERR(xdna, "Failed to pin pages ret %d", ret); > - goto destroy_pages; > - } > - } > + abo->type = AMDXDNA_BO_SHARE; [Severity: High] If a PASID client creates a UBUF BO and exports it via GEM_FLINK, and a non-PASID client then calls GEM_OPEN on it, the open path calls amdxdna_dma_map_bo(). Since the type is set to AMDXDNA_BO_SHARE here, the mapping routine invokes drm_gem_shmem_get_pages_sgt() for this object. Can this dereference an uninitialized shmem->base.filp and trigger a kernel WARN_ON and crash, given that UBUF BOs lack shmem initialization? > + 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); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
