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

[Anandu Krishnan E <anandu.e@oss.qualcomm.com>]

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
>>>>
>>>>