Re: [PATCH v4 19/25] accel/qda: Add remote memory map and unmap to the legacy shim

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] accel/qda: use-after-free race on placeholder in qda_shim_mem_map()
- [High] accel/qda: unmap size mismatch in qda_shim_mmap() when size is 0
- [High] accel/qda: missing unplug check in shim_mappings_release() on close
- [High] accel/qda: premature buffer release when legacy unmap fails or is interrupted
- [Medium] accel/qda: race in shim_mapping_detach() and shim_mapping_attach()
- [Medium] accel/qda: missing offset bounds check in setup_mmap_pages()
--

commit b1739b465f1ac98dac259c9a90724960699924f1
Author: Ekansh Gupta <ekangupt@qti.qualcomm.com>

accel/qda: Add remote memory map and unmap to the legacy shim

This commit implements the four legacy FastRPC memory map and unmap ioctls
in the QDA compatibility shim by importing DMA-BUF file descriptors and
tracking live mappings per open file so buffers remain referenced while
mapped on the DSP.

> diff --git a/drivers/accel/qda/qda_fastrpc.c b/drivers/accel/qda/qda_fastrpc.c
> index dbd14cd07cb20..b35604fce630c 100644
> --- a/drivers/accel/qda/qda_fastrpc.c
> +++ b/drivers/accel/qda/qda_fastrpc.c

[ ... ]

> @@ -935,19 +935,33 @@ static int qda_fastrpc_prepare_args_init_create(struct qda_fastrpc_invoke_ctx *c
>   * buffer is held by the context until the request is done, so that closing its
>   * handle meanwhile cannot free memory the DSP is being told to map. The
>   * region must fit in the buffer, as the DSP would otherwise map past its end.
> + *
> + * A caller with no handle table supplies the buffer already imported. It is
> + * checked as a buffer named by handle would be: an fd can name a buffer that
> + * another session exported, which resolves to the original object, still
> + * mapped in that session's context bank.
>   */
> -static int setup_mmap_pages(struct qda_fastrpc_invoke_ctx *ctx, struct drm_qda_mem_map *margs,
> +static int setup_mmap_pages(struct qda_fastrpc_invoke_ctx *ctx, struct qda_mem_map_args *args,
>  			    struct qda_phy_page *pages)
>  {
> +	struct drm_qda_mem_map *margs = &args->map;
>  	struct qda_gem_obj *qda_gem_obj;
>  	int err;
>  
> -	if (!margs->handle)
> -		return -EINVAL;
> +	if (args->obj) {
> +		if (to_qda_gem_obj(args->obj)->iommu_dev != ctx->session->assigned_iommu_dev)
> +			return -EACCES;
>  
> -	err = get_gem_obj_from_handle(ctx, margs->handle, &ctx->map_obj);
> -	if (err)
> -		return err;
> +		drm_gem_object_get(args->obj);
> +		ctx->map_obj = args->obj;
> +	} else {
> +		if (!margs->handle)
> +			return -EINVAL;
> +
> +		err = get_gem_obj_from_handle(ctx, margs->handle, &ctx->map_obj);
> +		if (err)
> +			return err;
> +	}
>  
>  	qda_gem_obj = to_qda_gem_obj(ctx->map_obj);
>  	if (margs->size > qda_gem_obj->size)
>  		return -EINVAL;
>  
>  	pages->addr = qda_gem_obj->dma_addr;
>  	pages->size = margs->size ? margs->size : qda_gem_obj->size;

[Severity: Medium]
Can a caller of FASTRPC_IOCTL_MEM_MAP or DRM_IOCTL_QDA_REMOTE_MMAP (with
QDA_REQUEST_MEM_MAP) map past the end of qda_gem_obj by supplying a
non-zero margs->offset?

In qda_shim_mem_map(), only args.offset < 0 is rejected, and
qda_fastrpc_prepare_args_mem_map() only checks margs->offset > INT_MAX
before setting req->offset = margs->offset.

Here in setup_mmap_pages(), only margs->size > qda_gem_obj->size is
checked. If a caller passes 0 < margs->offset <= INT_MAX and
margs->size <= qda_gem_obj->size (including margs->size == 0, which sets
pages->size to qda_gem_obj->size) such that margs->offset + pages->size
exceeds qda_gem_obj->size, shouldn't setup_mmap_pages() also check that
margs->offset <= qda_gem_obj->size and pages->size <= qda_gem_obj->size -
margs->offset?

> diff --git a/drivers/accel/qda/qda_fastrpc_shim.c b/drivers/accel/qda/qda_fastrpc_shim.c
> index 4a2ba20ee01ac..c46d2de4f5818 100644
> --- a/drivers/accel/qda/qda_fastrpc_shim.c
> +++ b/drivers/accel/qda/qda_fastrpc_shim.c

[ ... ]

> @@ -79,6 +101,83 @@ static struct drm_gem_object *shim_import_fd(struct qda_session *session, s32 fd
>  	return obj;
>  }
>  
> +/* Takes ownership of @obj on success; on failure the caller still owns it */
> +static struct qda_shim_mapping *shim_mapping_add(struct qda_shim_file *sf,
> +						 struct list_head *list, u64 key, u64 vaddr,
> +						 u64 size, struct drm_gem_object *obj)
> +{
> +	struct qda_shim_mapping *m, *new;
> +
> +	new = kzalloc_obj(*new);
> +	if (!new)
> +		return ERR_PTR(-ENOMEM);
> +
> +	new->key = key;
> +	new->vaddr = vaddr;
> +	new->size = size;
> +	new->obj = obj;
> +
> +	guard(mutex)(&sf->maps_lock);
> +
> +	list_for_each_entry(m, list, node) {
> +		if (m->key == key) {
> +			kfree(new);
> +			return ERR_PTR(-EEXIST);
> +		}
> +	}
> +	list_add_tail(&new->node, list);
> +
> +	return new;
> +}
> +
> +static struct qda_shim_mapping *shim_mapping_detach(struct qda_shim_file *sf,
> +						    struct list_head *list, u64 key, u64 vaddr)
> +{
> +	struct qda_shim_mapping *m;
> +
> +	guard(mutex)(&sf->maps_lock);
> +
> +	list_for_each_entry(m, list, node) {
> +		if (m->key == key && m->vaddr == vaddr) {
> +			list_del(&m->node);
> +			return m;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
> +static void shim_mapping_attach(struct qda_shim_file *sf, struct list_head *list,
> +				struct qda_shim_mapping *m)
> +{
> +	guard(mutex)(&sf->maps_lock);
> +	list_add_tail(&m->node, list);
> +}

[Severity: Medium]
Can detaching a mapping in shim_mapping_detach() and dropping sf->maps_lock
before validating size or calling the blocking unmap RPC race with
concurrent map and unmap calls on the same open file?

First, in qda_shim_mem_unmap(), m is removed from sf->mem_map_list while
qda_legacy_ops_table.mem_unmap() runs without sf->maps_lock held. While m
is detached, its key (args.fd) is absent from sf->mem_map_list, so a
concurrent qda_shim_mem_map() with the same fd can pass the duplicate-key
check in shim_mapping_add() and insert a second entry with the same fd. If
the in-flight mem_unmap() then fails (for example, interrupted by a signal
with -ERESTARTSYS in wait_for_completion_interruptible()),
shim_mapping_attach() unconditionally appends m back onto sf->mem_map_list
with list_add_tail(), leaving duplicate fd keys in sf->mem_map_list.

Second, in both qda_shim_mmap() and qda_shim_mem_map(), m is visible on the
list before copy_to_user() runs. If a concurrent qda_shim_munmap() (for
example, with a mismatched args.size since m->size != args.size is checked
after shim_mapping_detach()) or a failing qda_shim_mem_unmap() temporarily
detaches m while copy_to_user() faults, the error rollback call to
shim_mapping_detach() in the map path will return NULL and skip unmapping
and freeing m, after which the failing unmap path calls
shim_mapping_attach() and re-attaches m even though the map ioctl returned
-EFAULT.

> +
> +static void shim_mapping_free(struct qda_shim_mapping *m)
> +{
> +	drm_gem_object_put(m->obj);
> +	kfree(m);
> +}
> +
> +/* The DSP unmap comes first: the buffer must outlive the DSP's use of it */
> +static void shim_mappings_release(struct qda_shim_file *sf)
> +{
> +	struct qda_shim_mapping *m, *tmp;
> +
> +	list_for_each_entry_safe(m, tmp, &sf->mmap_list, node) {
> +		list_del(&m->node);
> +		qda_legacy_ops_table.munmap(sf->session, m->vaddr, m->size);
> +		shim_mapping_free(m);
> +	}
> +
> +	list_for_each_entry_safe(m, tmp, &sf->mem_map_list, node) {
> +		list_del(&m->node);
> +		qda_legacy_ops_table.mem_unmap(sf->session, m->key, m->vaddr, m->size);
> +		shim_mapping_free(m);
> +	}
> +}

[Severity: High]
What happens in qda_shim_release() -> shim_mappings_release() if the file
is closed after the underlying rpmsg device has been removed?

When the rpmsg device is removed while a /dev/fastrpc-* file is still open,
qda_rpmsg_remove() calls drm_dev_unplug(), qda_cb_unpopulate() (destroying
each context-bank platform device), and qda_deinit_device() (destroying
qdev->iommu_mgr->assignment_lock and qdev->ctx_xa), and devres frees
qdev->iommu_mgr.

In drivers/accel/qda/qda_legacy_ops.c, legacy_release() checks
drm_dev_is_unplugged(&session->qda_dev->drm_dev) before calling
qda_release_dsp_process(). However, qda_shim_release() calls
shim_mappings_release() before legacy_release(), and neither
shim_mappings_release() nor legacy_munmap() and legacy_mem_unmap() checks
drm_dev_is_unplugged(). Won't closing a file with active mappings after
unplug call into qda_fastrpc_invoke() -> xa_alloc(&qdev->ctx_xa, ...) and
alloc_kernel_gem() -> qda_memory_manager_alloc(), dereferencing the
destroyed ctx_xa, devm-freed qdev->iommu_mgr, and destroyed context-bank
platform device?

[ ... ]

> @@ -191,6 +295,156 @@ static long qda_shim_init_create(struct qda_shim_file *sf, void __user *argp)
>  	return err;
>  }
>  
> +static long qda_shim_mmap(struct qda_shim_file *sf, void __user *argp)
> +{
> +	struct qda_shim_mapping *m;
> +	struct fastrpc_req_mmap args;
> +	struct drm_gem_object *obj;
> +	int err;
> +
> +	if (copy_from_user(&args, argp, sizeof(args)))
> +		return -EFAULT;
> +
> +	obj = shim_import_fd(sf->session, args.fd);
> +	if (IS_ERR(obj))
> +		return PTR_ERR(obj);
> +	if (!obj)
> +		return -EINVAL;
> +
> +	err = qda_legacy_ops_table.mmap(sf->session, obj, args.vaddrin, args.size, args.flags,
> +					&args.vaddrout);
> +	if (err) {
> +		drm_gem_object_put(obj);
> +		return err;
> +	}
> +
> +	m = shim_mapping_add(sf, &sf->mmap_list, args.vaddrout, args.vaddrout, args.size, obj);

[Severity: High]
If user space passes args.size == 0 to FASTRPC_IOCTL_MMAP (or
args.length == 0 to FASTRPC_IOCTL_MEM_MAP in qda_shim_mem_map()), doesn't
this record m->size = 0 while the DSP actually mapped the full GEM object
size?

In setup_mmap_pages():

	pages->addr = qda_gem_obj->dma_addr;
	pages->size = margs->size ? margs->size : qda_gem_obj->size;

When args.size is 0, the DSP maps qda_gem_obj->size bytes, but
qda_shim_mmap() and qda_shim_mem_map() record 0 in m->size. When the
mapping is later unmapped via qda_shim_munmap(), qda_shim_mem_unmap(),
copy_to_user() failure rollback, or shim_mappings_release(), the DSP is
asked to unmap 0 bytes before shim_mapping_free() drops the reference on
obj, and FASTRPC_IOCTL_MUNMAP calls specifying the actual buffer size will
be rejected by m->size != args.size. Should m->size record the effective
mapped size (or should a zero size/length be rejected)?

> +	if (IS_ERR(m)) {
> +		err = PTR_ERR(m);
> +		goto err_unmap;
> +	}
> +
> +	if (copy_to_user(argp, &args, sizeof(args))) {
> +		m = shim_mapping_detach(sf, &sf->mmap_list, args.vaddrout, args.vaddrout);
> +		if (m) {
> +			qda_legacy_ops_table.munmap(sf->session, m->vaddr, m->size);
> +			shim_mapping_free(m);
> +		}
> +		return -EFAULT;
> +	}
> +
> +	return 0;
> +
> +err_unmap:
> +	qda_legacy_ops_table.munmap(sf->session, args.vaddrout, args.size);
> +	drm_gem_object_put(obj);
> +
> +	return err;
> +}

[Severity: High]
Can ignoring the return value of munmap() and mem_unmap() in these rollback
paths (and in shim_mappings_release()) release m->obj while the DSP still
has the buffer mapped?

Here in qda_shim_mmap() (on copy_to_user() failure and at err_unmap), in
qda_shim_mem_map() (on copy_to_user() failure), and in
shim_mappings_release() on file close, qda_legacy_ops_table.munmap() or
mem_unmap() waits in qda_rpmsg_wait_for_rsp() via
wait_for_completion_interruptible(&ctx->work).

If a signal is pending or arrives while waiting (returning -ERESTARTSYS),
or if allocating the unmap invocation context fails, the DSP has not
unmapped the buffer, yet shim_mapping_free(m) or drm_gem_object_put(obj)
immediately drops the reference on the GEM object (which for an imported
DMA-BUF in the shim is the sole reference, tearing down its IOMMU mapping
or freeing its DMA memory in qda_gem_free_object()).

[ ... ]

> +static long qda_shim_mem_map(struct qda_shim_file *sf, void __user *argp)
> +{
> +	struct qda_shim_mapping *m;
> +	struct fastrpc_mem_map args;
> +	struct drm_gem_object *obj;
> +	int err;
> +
> +	if (copy_from_user(&args, argp, sizeof(args)))
> +		return -EFAULT;
> +
> +	if (args.offset < 0)
> +		return -EINVAL;
> +
> +	obj = shim_import_fd(sf->session, args.fd);
> +	if (IS_ERR(obj))
> +		return PTR_ERR(obj);
> +	if (!obj)
> +		return -EINVAL;
> +
> +	/* Reserved before the DSP is asked, so the same fd cannot be mapped twice */
> +	m = shim_mapping_add(sf, &sf->mem_map_list, args.fd, 0, args.length, obj);
> +	if (IS_ERR(m)) {
> +		drm_gem_object_put(obj);
> +		return PTR_ERR(m);
> +	}
> +
> +	err = qda_legacy_ops_table.mem_map(sf->session, obj, args.flags, args.attrs, args.offset,
> +					   args.vaddrin, args.length, args.fd, &args.vaddrout);
> +	if (err) {
> +		m = shim_mapping_detach(sf, &sf->mem_map_list, args.fd, 0);
> +		if (m)
> +			shim_mapping_free(m);
> +		return err;
> +	}
> +
> +	scoped_guard(mutex, &sf->maps_lock)
> +		m->vaddr = args.vaddrout;

[Severity: Critical]
Can a concurrent FASTRPC_IOCTL_MEM_UNMAP with args.vaddr == 0 detach and
free the placeholder m while qda_legacy_ops_table.mem_map() is still
running?

In qda_shim_mem_map(), shim_mapping_add() inserts m into sf->mem_map_list
with m->key = args.fd and m->vaddr = 0, and transfers the caller's only
reference on obj into m->obj before dropping sf->maps_lock:

	m = shim_mapping_add(sf, &sf->mem_map_list, args.fd, 0, args.length, obj);
	...
	err = qda_legacy_ops_table.mem_map(sf->session, obj, ...);

While qda_legacy_ops_table.mem_map() is executing without sf->maps_lock, a
concurrent qda_shim_mem_unmap() on the same file with the same args.fd and
args.vaddr == 0 will match the placeholder in shim_mapping_detach():

	list_for_each_entry(m, list, node) {
		if (m->key == key && m->vaddr == vaddr) {
			list_del(&m->node);
			return m;
		}
	}

If qda_legacy_ops_table.mem_unmap() then succeeds, qda_shim_mem_unmap()
calls shim_mapping_free(m), which puts m->obj (potentially before
setup_mmap_pages() has called drm_gem_object_get(args->obj)) and frees m.
When qda_legacy_ops_table.mem_map() finishes and returns 0,
qda_shim_mem_map() writes m->vaddr = args.vaddrout to the freed m and
leaves the DSP mapping active with m->obj already released.

Conversely, if qda_legacy_ops_table.mem_map() fails while the concurrent
qda_shim_mem_unmap() has detached m, qda_shim_mem_map()'s cleanup
shim_mapping_detach() returns NULL, and if mem_unmap() then fails and calls
shim_mapping_attach(), the failed placeholder with vaddr == 0 is
re-attached to sf->mem_map_list, leaking m->obj until file close and
blocking future FASTRPC_IOCTL_MEM_MAP calls on that fd with -EEXIST.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-v4-v4-0-88f2f24a5f0b@qti.qualcomm.com?part=19