Re: [PATCH 1/3] accel/amdxdna: refuse an I/O memory mapping of an imported BO

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