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

[Jianping <jianping.li@oss.qualcomm.com>]

在 2026/7/30 12:34, Ekansh Gupta 写道:
> On 16-07-2026 17:02, Jianping Li wrote:
>> DMA handles passed as invoke arguments (scalars beyond nbufs) may refer
>> to the same dma_buf fd as an input/output buffer argument. Taking an
>> extra reference for such DMA handle maps leads to duplicate mappings and
>> an unbalanced reference count, since DMA handle maps are released
>> separately when the DSP returns the fd through the fdlist.
>>
>> Fix this by not taking an extra reference for DMA handle arguments
>> (take_ref = false) and tagging them with FASTRPC_MAP_DMA_HANDLE. As
>> these maps are borrowed references, fastrpc_get_args() re-validates the
>> map via fastrpc_map_lookup() before dereferencing it, so it is not used
>> after being freed. fastrpc_put_args() only releases maps flagged as
>> FASTRPC_MAP_DMA_HANDLE and clears the flag to guarantee the map is freed
>> exactly once.
>>
>> Also reject FASTRPC_MAP_DMA_HANDLE in fastrpc_req_mem_map(), since such
>> handles are already mapped implicitly during the remote invoke call and
>> must not be mapped again through the explicit MEM_MAP path.
>>
>> Fixes: 10df039834f84 ("misc: fastrpc: Skip reference for DMA handles")
>> Cc: stable@kernel.org
>> Signed-off-by: Jianping Li <jianping.li@oss.qualcomm.com>
>> ---
>> Patch [v1]: https://lore.kernel.org/all/20260625080832.17477-1-jianping.li@oss.qualcomm.com/
>>
>> Changes in v2:
>> - Rework the commit message to describe the DMA handle reference and
>>    lifetime problem more precisely.
>> - Introduce a new FASTRPC_MAP_DMA_HANDLE uapi flag and a 'flags' field
>>    in struct fastrpc_map to explicitly tag DMA handle maps, instead of
>>    relying only on the nbufs boundary / take_ref.
>> - Plumb an mflags argument through fastrpc_map_create() and
>>    fastrpc_map_attach() so DMA handle maps are tagged at creation time.
>> - Re-validate the borrowed map in fastrpc_get_args() via
>>    fastrpc_map_lookup() before dereferencing it, to avoid a
>>    use-after-free when the map was created with take_ref = false.
>> - In fastrpc_put_args(), only release maps tagged FASTRPC_MAP_DMA_HANDLE
>>    and clear the flag afterwards, so such maps are freed exactly once.
>> - Reject FASTRPC_MAP_DMA_HANDLE in fastrpc_req_mem_map(), since these
>>    handles are already mapped implicitly during the remote invoke and
>>    must not be mapped again through the explicit MEM_MAP path.
>> ---
>>   drivers/misc/fastrpc.c      | 56 ++++++++++++++++++++++++++-----------
>>   include/uapi/misc/fastrpc.h |  2 ++
>>   2 files changed, 42 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index d86e79134c68..fa8c58a97e35 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -223,6 +223,7 @@ struct fastrpc_map {
>>   	u64 len;
>>   	u64 raddr;
>>   	u32 attr;
>> +	u32 flags;
>>   	struct kref refcount;
>>   };
>>   
>> @@ -833,7 +834,7 @@ static dma_addr_t fastrpc_compute_dma_addr(struct fastrpc_user *fl, dma_addr_t s
>>   }
>>   
>>   static int fastrpc_map_attach(struct fastrpc_user *fl, int fd,
>> -			      u64 len, u32 attr, struct fastrpc_map **ppmap)
>> +			      u64 len, u32 attr, struct fastrpc_map **ppmap, int mflags)
>>   {
>>   	struct fastrpc_session_ctx *sess = fl->sctx;
>>   	struct fastrpc_map *map = NULL;
>> @@ -850,6 +851,7 @@ static int fastrpc_map_attach(struct fastrpc_user *fl, int fd,
>>   
>>   	map->fl = fl;
>>   	map->fd = fd;
>> +	map->flags = mflags;
>>   	map->buf = dma_buf_get(fd);
>>   	if (IS_ERR(map->buf)) {
>>   		err = PTR_ERR(map->buf);
>> @@ -924,13 +926,13 @@ static int fastrpc_map_attach(struct fastrpc_user *fl, int fd,
>>   	return err;
>>   }
>>   
>> -static int fastrpc_map_create(struct fastrpc_user *fl, int fd,
>> -			      u64 len, u32 attr, struct fastrpc_map **ppmap)
>> +static int fastrpc_map_create(struct fastrpc_user *fl, int fd, u64 len, u32 attr,
>> +			      struct fastrpc_map **ppmap, bool take_ref, int mflags)
>>   {
>> -	if (!fastrpc_map_lookup(fl, fd, ppmap, true))
>> +	if (!fastrpc_map_lookup(fl, fd, ppmap, take_ref))
>>   		return 0;
>>   
>> -	return fastrpc_map_attach(fl, fd, len, attr, ppmap);
>> +	return fastrpc_map_attach(fl, fd, len, attr, ppmap, mflags);
>>   }
>>   
>>   /*
>> @@ -1000,23 +1002,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);
>>   		if (err) {
>>   			dev_err(dev, "Error Creating map %d\n", err);
>>   			return -EINVAL;
>>   		}
>> -
>>   	}
>> +
>>   	return 0;
>>   }
>>   
>> @@ -1144,6 +1148,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;
>> +			}
> if map is created by R1 call with DMA handle flag, when any inbuf/outbuf
> carries the same fd for R2 call, wouldn't the refcount get increased as
> part of fastrpc_create_maps()? So if the refcount is already increased
> there, why do you need to have a check here?>  			pages[i].addr =
> ctx->maps[i]->dma_addr;
The check is not for the case you described (same fd reused as an in/out 
buffer in a later call). In that case i < nbufs so the map is created 
with take_ref = true and the refcount is indeed held, and the re-lookup 
simply passes.
The check is needed for DMA handle args themselves, which are created 
with take_ref = false. For those maps this context holds only a borrowed 
reference — the owning reference is dropped when the DSP returns the fd 
via fdlist in fastrpc_put_args(), which may run from a concurrent 
invocation. So between fastrpc_create_maps() and fastrpc_get_args() the 
map may already have been freed, and dereferencing ctx->maps[i] would be 
a use-after-free. The re-lookup re-validates that the map is still 
present before we dereference it.
(If you believe there is no competition, I will drop this code.)
>>   			pages[i].size = ctx->maps[i]->size;
>>   		}
>> @@ -1200,8 +1214,13 @@ 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))
>> +		/* Validate the map flags for DMA handles and skip freeing map if invalid */
>> +		if (!fastrpc_map_lookup(fl, (int)fdlist[i], &mmap, false) &&
>> +		    mmap->flags == FASTRPC_MAP_DMA_HANDLE) {
>> +			/* Allow DMA handle maps to free only once */
>> +			mmap->flags = 0;
> flags are getting updated without any locks, this is considering that
> DSP can updated a particular fd on fdlist only once. Can you add this
> information here?>  			fastrpc_map_put(mmap);
Agreed, this relies on the DSP reporting a given fd in the fdlist only 
once, so no concurrent fastrpc_put_args() can update the same map's 
flags. I'll add a comment documenting this assumption in v3.
Will send v3 with the clarified comments.
>> +		}
>>   	}
>>   
>>   	return ret;
>> @@ -1511,7 +1530,7 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>>   	fl->pd = USER_PD;
>>   
>>   	if (init.filelen && init.filefd) {
>> -		err = fastrpc_map_create(fl, init.filefd, init.filelen, 0, &map);
>> +		err = fastrpc_map_create(fl, init.filefd, init.filelen, 0, &map, true, 0);
>>   		if (err)
>>   			goto err;
>>   	}
>> @@ -2107,9 +2126,14 @@ static int fastrpc_req_mem_map(struct fastrpc_user *fl, char __user *argp)
>>   
>>   	if (copy_from_user(&req, argp, sizeof(req)))
>>   		return -EFAULT;
>> -
>> +	/*
>> +	 * Prevent mapping backward compatible DMA handles here, as they are
>> +	 * already mapped in the remote call.
>> +	 */
>> +	if (req.flags == FASTRPC_MAP_DMA_HANDLE)
>> +		return -EINVAL;
>>   	/* create SMMU mapping */
>> -	err = fastrpc_map_create(fl, req.fd, req.length, 0, &map);
>> +	err = fastrpc_map_create(fl, req.fd, req.length, 0, &map, true, 0);
>>   	if (err) {
>>   		dev_err(dev, "failed to map buffer, fd = %d\n", req.fd);
>>   		return err;
>> diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
>> index c6e2925f47e6..142ddaeed85f 100644
>> --- a/include/uapi/misc/fastrpc.h
>> +++ b/include/uapi/misc/fastrpc.h
>> @@ -44,6 +44,8 @@ enum fastrpc_map_flags {
>>   	FASTRPC_MAP_FD = 2,
>>   	FASTRPC_MAP_FD_DELAYED,
>>   	FASTRPC_MAP_FD_NOMAP = 16,
>> +	/* Map the DMA handle in the invoke call for backward compatibility */
>> +	FASTRPC_MAP_DMA_HANDLE = 0x20000,
>>   	FASTRPC_MAP_MAX,
>>   };
>>   
>