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

[Jianping]

在 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@xxxxxxxxxx
> > Signed-off-by: Jianping Li <jianping.li@xxxxxxxxxxxxxxxx>
> > ---
> > Patch [v1]: https://lore.kernel.org/all/20260625080832.17477-1-jianping.li@xxxxxxxxxxxxxxxx/
> >
> > 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,
> >   };
> >