Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] Missing `amdxdna_gem_put_obj()` on the error path in `amdxdna_cmd_set_error` leaks a GEM object reference. -- --- Patch [1]: [PATCH 1/3] accel/amdxdna: refuse an I/O memory mapping of an imported BO --- commit c573f7ee611c1dede95280b62a62d7abef0e2670 Author: Taimuraz Kaitmazov <[email protected]> accel/amdxdna: refuse an I/O memory mapping of an imported BO amdxdna_gem_obj_vmap() accepts whatever dma_buf_vmap() returns and only rejects a NULL vaddr. struct iosys_map is a union discriminated by is_iomem, so an exporter that answers with an I/O mapping leaves a void __iomem pointer in map->vaddr, and amdxdna_gem_vmap() stores it in abo->mem.kva, which callers use as an ordinary kernel address: amdxdna_cmd_set_error() memsets and memcpys through it. > diff --git a/drivers/accel/amdxdna/amdxdna_gem.c > b/drivers/accel/amdxdna/amdxdna_gem.c > index 1f190b319bb6e..b66ec9e482894 100644 > --- a/drivers/accel/amdxdna/amdxdna_gem.c > +++ b/drivers/accel/amdxdna/amdxdna_gem.c > @@ -683,10 +683,16 @@ static int amdxdna_gem_obj_vmap(struct drm_gem_object > *obj, struct iosys_map *ma > > dma_resv_assert_held(obj->resv); > > - if (is_import_bo(abo)) > + if (is_import_bo(abo)) { > ret = dma_buf_vmap(abo->dma_buf, map); > - else > + /* Callers use mem.kva as an ordinary kernel address. */ > + if (!ret && map->is_iomem) { > + dma_buf_vunmap(abo->dma_buf, map); > + return -EOPNOTSUPP; > + } [Severity: High] This isn't a bug introduced by this patch, but refusing the mapping here exposes a reference leak in amdxdna_cmd_set_error(). If unprivileged userspace imports an IOMEM DMABUF and submits it as an ERT_CMD_CHAIN command that causes an error, we reach this code in drivers/accel/amdxdna/amdxdna_ctx.c: if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN) { cc = amdxdna_cmd_get_payload(abo, NULL); cc->error_index = (cmd_idx < cc->command_count) ? cmd_idx : 0; abo = amdxdna_gem_get_obj(client, cc->data[0], AMDXDNA_BO_SHARE); if (!abo) return -EINVAL; cmd = amdxdna_gem_vmap(abo); if (!cmd) return -ENOMEM; } Because amdxdna_gem_vmap() will now return NULL for the IOMEM BO, the code returns -ENOMEM immediately. Does this leak the GEM object reference acquired by amdxdna_gem_get_obj() since it bypasses the amdxdna_gem_put_obj() cleanup located at the end of the function? > + } else { > ret = drm_gem_shmem_object_vmap(obj, map); > + } > if (ret) > return ret; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
