Re: [PATCH v1] misc: fastrpc: Add reference counting for fastrpc_user structure

[Bjorn Andersson]

On Fri, Feb 27, 2026 at 07:52:00PM +0530, Anandu Krishnan E wrote:
> 
> 
> On 2/26/2026 11:20 PM, Bjorn Andersson wrote:
> > On Thu, Feb 26, 2026 at 08:41:21PM +0530, Anandu Krishnan E wrote:
> > > Add reference counting using kref to the fastrpc_user structure to
> > > prevent use-after-free issues when contexts are freed from workqueue
> > > after device release.
> > Please follow
> > https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
> > and start your commit message by clearly establishing the problem, once
> > that's done you can describe the technical solution.
> sure,  will update the commit message and send as patch v2.
> > 
> > > The issue occurs when fastrpc_device_release() frees the user structure
> > > while invoke contexts are still pending in the workqueue. When the
> > > workqueue later calls fastrpc_context_free(), it attempts to access
> > > buf->fl->cctx in fastrpc_buf_free(), leading to a use-after-free:
> > But why does it do that?
> > 
> > The reason why we need buf->fl->cctx in this context is because we need
> > to mask out the DMA address from the buf->dma_addr (remove the SID bits).
> > 
> > If we instead split "dma_addr" into two separate members of struct
> > fastrpc_buf, one dma_addr_t dma_addr and one u64 iova_with_sid (?), then
> > it would be clear throughout the driver which address space we're
> > talking about (is it the SID-adjusted iova space or the dma_addr_t in
> > the particular DMA context).
> > 
> > In addition to making this aspect of the driver easier to follow, you no
> > longer need to call fastrpc_ipa_to_dma_addr() in fastrpc_buf_free() (or
> > anywhere else for that matter).
> > 
> > You can just pass buf->dma_addr directly to dma_free_coherent().
> > 
> > You're isolating the fact that the firmware needs to see "SID |
> > dma_addr" to just two places in the driver.
> I agree with the refactoring direction you’re suggesting, and
> separating the address spaces does make the driver easier
> to reason about.
> 
> That said, the UAF isn’t limited to the buffer
> free path. fastrpc_context_free() also calls fastrpc_map_put(),
> which reaches fastrpc_free_map() and still dereferences fl, so
> addressing only the buffer side doesn’t fully resolve the lifetime issue.
> So the reference counting is still needed. I will update the scenario in
> commit message as well.
> 

I presume you're referring to the "vmid" we need to rebuild the
src_perms for use in fastrpc_free_map()?

I think the relevant question to ask there is if it's really a property
of the "fastrpc file context". It seems to me that we could solve that
by storing the src_perms in the fastrpc_map once we've done the
qcom_scm_assign_mem() call in fastrpc_map_attach() - so that we can free
that object without having to reach out to objects of other lifetimes.

> If you think it makes sense, I can take the address‑space refactoring
> as a separate follow‑up patch and keep this change focused on fixing
> the lifetime issue.

The chance of you fixing one lifetime issue by introducing one or more
worries me, I'm only familiar with the driver, so I wouldn't be able to
say with confidence without investing more time fully understand the
various lifetimes. So if we're going that path, I'd like someone else to
step up and tell me that it's good.

On the other hand, the two changes I presented above are logically
simple to make, follow, and review - and they don't complicate the
driver further. So that would still be _my_ preferred choice.

[..]
> > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> > > index 47356a5d5804..3ababcf327d7 100644
> > > --- a/drivers/misc/fastrpc.c
> > > +++ b/drivers/misc/fastrpc.c
> > > @@ -310,6 +310,8 @@ struct fastrpc_user {
> > >   	spinlock_t lock;
> > >   	/* lock for allocations */
> > >   	struct mutex mutex;
> > > +	/* Reference count */
> > > +	struct kref refcount;
> > >   };
> > >   /* Extract SMMU PA from consolidated IOVA */
> > > @@ -497,15 +499,36 @@ static void fastrpc_channel_ctx_put(struct fastrpc_channel_ctx *cctx)
> > >   	kref_put(&cctx->refcount, fastrpc_channel_ctx_free);
> > >   }
> > > +static void fastrpc_user_free(struct kref *ref)
> > > +{
> > > +	struct fastrpc_user *fl = container_of(ref, struct fastrpc_user, refcount);
> > Unrelated question, what does the "fl" abbreviation actually mean? I
> > presume 'f' is for "fastrpc", but what is 'l'?
> fl is short for fastrpc file.
> 

Hmm, okay. Thank you for clarifying.

Regards,
Bjorn

> Regards,
> Anandu
> > 
> > Regards,
> > Bjorn
> > > +
> > > +	fastrpc_channel_ctx_put(fl->cctx);
> > > +	mutex_destroy(&fl->mutex);
> > > +	kfree(fl);
> > > +}
> > > +
> > > +static void fastrpc_user_get(struct fastrpc_user *fl)
> > > +{
> > > +	kref_get(&fl->refcount);
> > > +}
> > > +
> > > +static void fastrpc_user_put(struct fastrpc_user *fl)
> > > +{
> > > +	kref_put(&fl->refcount, fastrpc_user_free);
> > > +}
> > > +
> > >   static void fastrpc_context_free(struct kref *ref)
> > >   {
> > >   	struct fastrpc_invoke_ctx *ctx;
> > >   	struct fastrpc_channel_ctx *cctx;
> > > +	struct fastrpc_user *fl;
> > >   	unsigned long flags;
> > >   	int i;
> > >   	ctx = container_of(ref, struct fastrpc_invoke_ctx, refcount);
> > >   	cctx = ctx->cctx;
> > > +	fl = ctx->fl;
> > >   	for (i = 0; i < ctx->nbufs; i++)
> > >   		fastrpc_map_put(ctx->maps[i]);
> > > @@ -521,6 +544,8 @@ static void fastrpc_context_free(struct kref *ref)
> > >   	kfree(ctx->olaps);
> > >   	kfree(ctx);
> > > +	/* Release the reference taken in fastrpc_context_alloc() */
> > > +	fastrpc_user_put(fl);
> > >   	fastrpc_channel_ctx_put(cctx);
> > >   }
> > > @@ -628,6 +653,8 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
> > >   	/* Released in fastrpc_context_put() */
> > >   	fastrpc_channel_ctx_get(cctx);
> > > +	/* Take a reference to user, released in fastrpc_context_free() */
> > > +	fastrpc_user_get(user);
> > >   	ctx->sc = sc;
> > >   	ctx->retval = -1;
> > > @@ -658,6 +685,7 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
> > >   	spin_lock(&user->lock);
> > >   	list_del(&ctx->node);
> > >   	spin_unlock(&user->lock);
> > > +	fastrpc_user_put(user);
> > >   	fastrpc_channel_ctx_put(cctx);
> > >   	kfree(ctx->maps);
> > >   	kfree(ctx->olaps);
> > > @@ -1606,11 +1634,9 @@ static int fastrpc_device_release(struct inode *inode, struct file *file)
> > >   	}
> > >   	fastrpc_session_free(cctx, fl->sctx);
> > > -	fastrpc_channel_ctx_put(cctx);
> > > -
> > > -	mutex_destroy(&fl->mutex);
> > > -	kfree(fl);
> > >   	file->private_data = NULL;
> > > +	/* Release the reference taken in fastrpc_device_open */
> > > +	fastrpc_user_put(fl);
> > >   	return 0;
> > >   }
> > > @@ -1654,6 +1680,7 @@ static int fastrpc_device_open(struct inode *inode, struct file *filp)
> > >   	spin_lock_irqsave(&cctx->lock, flags);
> > >   	list_add_tail(&fl->user, &cctx->users);
> > >   	spin_unlock_irqrestore(&cctx->lock, flags);
> > > +	kref_init(&fl->refcount);
> > >   	return 0;
> > >   }
> > > -- 
> > > 2.34.1
> > > 
> > > 
>