Re: [PATCH v4] misc: fastrpc: avoid duplicate DMA mappings in fastrpc_create_maps()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Critical] Time-of-Check to Time-of-Use (TOCTOU) Use-After-Free and premature SMMU unmapping due to missing reference counting for DMA handles.
- [Critical] Data Race in `fastrpc_put_args()` leading to a Double Free of `fastrpc_map` objects.

Pre-existing issues:
- [High] Permanent Memory and IOMMU Leak of DMA handle maps on invocation error paths.
--

commit de606379cd29bd912e7ec93a58384eaa07c8ecd6
Author: Jianping Li <jianping.li@oss.qualcomm.com>

misc: fastrpc: avoid duplicate DMA mappings in fastrpc_create_maps()

This patch attempts to fix duplicate DMA mappings by not taking an extra
reference for DMA handle arguments (take_ref = false). It tags them with
FASTRPC_MAP_DMA_HANDLE, re-validates the map in fastrpc_get_args() before
use, and relies on fastrpc_put_args() to release these tagged maps when
the DSP returns the fd.

Link: https://lore.kernel.org/all/20260805060940.41414-1-jianping.li@oss.qualcomm.com/

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index d4fac2caca867..58f27e2713172 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c

[ ... ]

> @@ -1047,23 +1052,25 @@ static int fastrpc_create_maps(struct fastrpc_invoke_ctx *ctx)
>  	int i, err;
>  
>  	for (i = 0; i < ctx->nscalars; ++i) {
> +		bool take_ref = i < ctx->nbufs;
> +		int mflags = 0;
>  
>  		if (ctx->args[i].fd == 0 || ctx->args[i].fd == -1 ||
>  		    ctx->args[i].length == 0)
>  			continue;
>  
> -		if (i < ctx->nbufs)
> -			err = fastrpc_map_create(ctx->fl, ctx->args[i].fd,
> -				 ctx->args[i].length, ctx->args[i].attr, &ctx->maps[i]);
> -		else
> -			err = fastrpc_map_attach(ctx->fl, ctx->args[i].fd,
> -				 ctx->args[i].length, ctx->args[i].attr, &ctx->maps[i]);
> +		/* Set the DMA handle mapping flag for DMA handles */
> +		if (i >= ctx->nbufs)
> +			mflags = FASTRPC_MAP_DMA_HANDLE;
> +
> +		err = fastrpc_map_create(ctx->fl, ctx->args[i].fd, ctx->args[i].length,
> +					 ctx->args[i].attr, &ctx->maps[i], take_ref, mflags);

[Severity: High]
This is a pre-existing issue, but does fastrpc_create_maps() leak the DMA
handle maps on error paths?

If this function successfully creates a DMA handle map (where i >=
ctx->nbufs), but a subsequent step in fastrpc_internal_invoke() fails, the
cleanup path in fastrpc_context_free() is triggered.

fastrpc_context_free() only iterates up to nbufs:

    for (i = 0; i < ctx->nbufs; i++)
        fastrpc_map_put(ctx->maps[i]);

Since the cleanup loop ignores indices >= nbufs, and the invoke never
reaches the DSP to trigger fastrpc_put_args(), do these maps and their
SMMU mappings leak permanently?

>  		if (err) {
>  			dev_err(dev, "Error Creating map %d\n", err);
>  			return -EINVAL;
>  		}
> -
>  	}
> +
>  	return 0;
>  }

[ ... ]

> @@ -1195,6 +1202,16 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
>  		list[i].num = ctx->args[i].length ? 1 : 0;
>  		list[i].pgidx = i;
>  		if (ctx->maps[i]) {
> +			/* It is possible that map is created with
> +			 * mflags FASTRPC_MAP_DMA_HANDLE and take_ref
> +			 * is false. Check if map still exists or is
> +			 * being freed as take_ref is false
> +			 */
> +			if (fastrpc_map_lookup(ctx->fl, ctx->args[i].fd,
> +					       &ctx->maps[i], false)) {
> +				ctx->maps[i] = NULL;
> +				return -EINVAL;
> +			}
>  			pages[i].addr = ctx->maps[i]->dma_addr;

[Severity: Critical]
Can this lockless dereference lead to a use-after-free regression?

Since fastrpc_create_maps() maps the DMA handle with take_ref = false,
the shared map's refcount remains 1. If a concurrent invocation completes
and executes fastrpc_put_args() exactly after fastrpc_map_lookup() drops
the lock but before ctx->maps[i]->dma_addr is evaluated, it will clear the
flag and free the map.

Would this result in a host kernel use-after-free and premature SMMU
unmapping when the unreferenced pointer is accessed?

>  			pages[i].size = ctx->maps[i]->size;
>  		}

[ ... ]

> @@ -1244,8 +1261,17 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
>  	for (i = 0; i < FASTRPC_MAX_FDLIST; i++) {
>  		if (!fdlist[i])
>  			break;
> -		if (!fastrpc_map_lookup(fl, (int)fdlist[i], &mmap, false))
> +		/*
> +		 * DMA handle maps are released when the DSP returns the corresponding fd in
> +		 * fdlist. The DSP is expected to return a specific fd only once in fdlist,
> +		 * so no two fastrpc_put_args() paths should clear the DMA_HANDLE flag for
> +		 * the same map concurrently.
> +		 */
> +		if (!fastrpc_map_lookup(fl, (int)fdlist[i], &mmap, false) &&
> +		    mmap->flags == FASTRPC_MAP_DMA_HANDLE) {
> +			mmap->flags = 0;
>  			fastrpc_map_put(mmap);
> +		}

[Severity: Critical]
Does clearing mmap->flags outside a lock introduce a race condition leading
to a double free regression?

If userspace submits two concurrent invocations sharing the same fd, and
the DSP returns the fd in the fdlist of both, two threads executing
fastrpc_put_args() could look up the shared map locklessly.

Thread 1: evaluates mmap->flags == FASTRPC_MAP_DMA_HANDLE as true
Thread 2: evaluates mmap->flags == FASTRPC_MAP_DMA_HANDLE as true
Thread 1: clears flags and calls fastrpc_map_put() freeing the map
Thread 2: clears flags and calls fastrpc_map_put() on a freed pointer

Can this lead to host kernel heap corruption if triggered by concurrent
invokes?

>  	}
>  
>  	return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914104019.492-1-jianping.li@oss.qualcomm.com?part=1