Re: [PATCH v3 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] dma_buf_vmap() is called without holding the required dma_resv lock.
--

--- Patch [1]: [PATCH v3 1/3] accel/amdxdna: refuse an I/O memory mapping of an imported BO ---
commit a77f8bd3c9728d98291116a86706c7b30d341618
Author: Taimuraz Kaitmazov <[email protected]>

accel/amdxdna: refuse an I/O memory mapping of an imported BO

This patch modifies amdxdna_gem_obj_vmap() to refuse I/O memory mappings
from imported buffer objects. If an exporter returns an IOMEM mapping,
the mapping is rejected with -EOPNOTSUPP. This prevents invalid memory
accesses when later code assumes a regular kernel virtual address.

[ ... ]

> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index b01f5d95fac83..d7d36c9523253 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
> @@ -710,10 +710,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);

[Severity: High]
This is a pre-existing issue, but while looking at dma_buf_vmap() usage,
I noticed a missing lock in drivers/accel/amdxdna/amdxdna_cbuf.c.

Is dma_buf_vmap() being called without holding the required dma_resv lock
during carveout initialization?

When amdxdna_get_cbuf() exports a new dma_buf, it returns an unlocked buffer.
It then immediately calls amdxdna_cbuf_clear(), which invokes dma_buf_vmap()
on the dbuf without taking its reservation lock:

drivers/accel/amdxdna/amdxdna_cbuf.c:amdxdna_cbuf_clear() {
	...
	struct iosys_map vmap = IOSYS_MAP_INIT_VADDR(NULL);

	dma_buf_vmap(dbuf, &vmap);
	if (!vmap.vaddr)
		return -EFAULT;
	...
}

Because dma_buf_vmap() unconditionally asserts that this lock is held
(dma_resv_assert_held(dmabuf->resv)), does this unlocked call violate the
dma-buf API contract and produce kernel warnings?

> -	else
> +		/* Callers use mem.kva as an ordinary kernel address. */
> +		if (!ret && map->is_iomem) {
> +			dma_buf_vunmap(abo->dma_buf, map);
> +			return -EOPNOTSUPP;
> +		}
> +	} else {
>  		ret = drm_gem_shmem_object_vmap(obj, map);
> +	}
>  	if (ret)
>  		return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1