Re: [PATCH v2] misc: fastrpc: avoid duplicate DMA mappings in fastrpc_create_maps()
From: Ekansh Gupta
Date: Thu Jul 30 2026 - 23:28:10 EST
On 30-07-2026 13:53, Jianping wrote:
>
>
> 在 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.)
Okay, I see this is only for DMA handles. Thanks for the
explaination.>>> 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.
Thanks!
//ekansh>>> + }
>>> }
>>> 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,
>>> };
>>>
>>
>