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

[Anandu Krishnan E]

On 2/28/2026 12:44 AM, Bjorn Andersson wrote:
> 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.
I understand your concerns about the ref count approach.
But with the two above changes also UAF issue will still persist.
In fastrpc_free_map() :

if (map->fl) {
    spin_lock(&map->fl->lock);
    list_del(&map->node);
    spin_unlock(&map->fl->lock);
    map->fl = NULL;
}

we are using above logic to remove map node from the list.
Here also we are using fl->lock to manage the map list and
this map is part of fl->maps list as well.


I suggested ref count change because it was handling all the
scenarios we discussed above. I am open to any alternative
suggestions as well.

Regards,
Anandu

> [..]
> > > > 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