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

[Srinivas Kandagatla]

On 2/26/26 3:11 PM, 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.
> 
> 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:

I guess these are some of the pending invoke requests that are still
processing and receive callback after user closes the device.

> 
>   pc : fastrpc_buf_free+0x38/0x80 [fastrpc]
>   lr : fastrpc_context_free+0xa8/0x1b0 [fastrpc]
>   ...
>   fastrpc_context_free+0xa8/0x1b0 [fastrpc]
>   fastrpc_context_put_wq+0x78/0xa0 [fastrpc]
>   process_one_work+0x180/0x450
>   worker_thread+0x26c/0x388
> 
> Implement proper reference counting to fix this:
> - Initialize kref in fastrpc_device_open()
> - Take a reference in fastrpc_context_alloc() for each context
> - Release the reference in fastrpc_context_free() when context is freed
> - Release the initial reference in fastrpc_device_release()
> 
> This ensures the user structure remains valid as long as there are
> contexts holding references to it, preventing the race condition.
> 
> Fixes: 6cffd79504ce ("misc: fastrpc: Add support for dmabuf exporter")
> Cc: stable@xxxxxxxxxx
> Signed-off-by: Anandu Krishnan E <anandu.e@xxxxxxxxxxxxxxxx>
> ---
>  drivers/misc/fastrpc.c | 35 +++++++++++++++++++++++++++++++----
>  1 file changed, 31 insertions(+), 4 deletions(-)
> 
> 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);
> +
> +	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);

Now that we are trying to handle(wait) for the cases where wq will get
callbacks after the fd is closed, how are we making sure that memory
associated with these invoke, init memory and mmaps are not released?



Also now that we have a refcount for the fastrpc_user, is it possible to
 remove the refcount on the context, as this seems redundant now, we
could just use the user refcount instead. May be other patch..


--srini
>  
>  	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;
>  }