Re: [PATCH v2] misc: fastrpc: fix context leak and hang on signal-interrupted invoke

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Restoring an interrupted context fails to increment its reference count, leading to a Use-After-Free and Double Free when the syscall completes.
- [Critical] Retrying an interrupted fastrpc invocation with a newly allocated arguments structure forces the kernel to blindly write output data to old, potentially invalidated buffer addresses.
- [High] If a thread's fastrpc call is interrupted and it subsequently attempts a completely different fastrpc call, it will be permanently blocked from using the device.
- [High] Double-scheduling `ctx->put_work` in `fastrpc_notify_users()` causes list corruption and a Use-After-Free if the async worker is already executing.
--

commit d067e3f966edecc9e2338c8eea087bb05365a462
Author: Anandu Krishnan E <anandu.e@oss.qualcomm.com>

misc: fastrpc: fix context leak and hang on signal-interrupted invoke

This commit aims to fix a context leak and hang when a fastrpc invocation is
interrupted by a signal. It saves the interrupted context to a new list and
restores it on the next syscall attempt.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index f3a49384586d1..c0859a5016179 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
[ ... ]
> @@ -594,6 +605,42 @@ static void fastrpc_context_put_wq(struct work_struct *work)
>  	fastrpc_context_put(ctx);
>  }
>  
> +static void fastrpc_context_save_interrupted(struct fastrpc_invoke_ctx *ctx)
> +{
> +	spin_lock(&ctx->fl->lock);
> +	list_del(&ctx->node);
> +	list_add_tail(&ctx->node, &ctx->fl->interrupted);
> +	spin_unlock(&ctx->fl->lock);
> +	/*
> +	 * invoke_send bumped the kref to 2; the bail path skips the put
> +	 * for ERESTARTSYS.  Drop it here so the worker's put reaches 0
> +	 * and triggers context_free.
> +	 */
> +	fastrpc_context_put(ctx);
> +}
> +
> +static struct fastrpc_invoke_ctx *fastrpc_context_restore_interrupted(
> +			struct fastrpc_user *fl, u32 sc)
> +{
> +	struct fastrpc_invoke_ctx *ctx = NULL, *ictx, *n;
> +
> +	spin_lock(&fl->lock);
> +	list_for_each_entry_safe(ictx, n, &fl->interrupted, node) {
> +		if (ictx->pid != current->pid)
> +			continue;
> +		if (ictx->sc != sc || ictx->fl != fl) {
> +			spin_unlock(&fl->lock);
> +			return ERR_PTR(-EINVAL);

[Severity: High]
If a thread gets interrupted and attempts a new fastrpc call with a different
sc, this returns -EINVAL but leaves the old context on fl->interrupted.

Will every subsequent fastrpc call by this thread with a different sc hit this
same mismatch and instantly fail with -EINVAL, effectively wedging the thread?

> +		}
> +		ctx = ictx;
> +		list_del(&ctx->node);
> +		list_add_tail(&ctx->node, &fl->pending);
> +		break;

[Severity: Critical]
Does restoring the context here require incrementing its reference count via
fastrpc_context_get()?

Since the invoker's reference is intentionally dropped in
fastrpc_context_save_interrupted(), if the async worker completes the work and
drops its reference, could the user thread wake up and unconditionally call
fastrpc_context_put() in the bail path on freed memory?

> +	}
> +	spin_unlock(&fl->lock);
> +	return ctx;
> +}
> +
[ ... ]
> @@ -1258,6 +1303,14 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel,
>  		return -EPERM;
>  	}
>  
> +	if (!kernel) {
> +		ctx = fastrpc_context_restore_interrupted(fl, sc);
> +		if (IS_ERR(ctx))
> +			return PTR_ERR(ctx);
> +		if (ctx)
> +			goto wait;

[Severity: Critical]
By jumping directly to wait, does this bypass argument extraction for retries?

If a user thread unwinds its stack after a signal and initiates a completely
different call that happens to have the same sc, it seems the output data
might be copied directly to the old buffer addresses stored in ctx->args,
which could now contain other sensitive data.

> +	}
> +
>  	ctx = fastrpc_context_alloc(fl, kernel, sc, args);
>  	if (IS_ERR(ctx))
>  		return PTR_ERR(ctx);
[ ... ]
> @@ -2493,13 +2549,23 @@ static void fastrpc_notify_users(struct fastrpc_user *user)
>  		ctx->retval = -EPIPE;
>  		complete(&ctx->work);
>  	}
> +	/*
> +	 * Interrupted contexts hold two refs: one for the invoker and one
> +	 * for the async worker from fastrpc_invoke_send.  Complete them so
> +	 * any blocked retry wakes, and schedule put_wq to drop the worker
> +	 * ref -- the invoker ref is released by fastrpc_user_free().
> +	 */
> +	list_for_each_entry(ctx, &user->interrupted, node) {
> +		ctx->retval = -EPIPE;
> +		complete(&ctx->work);
> +		schedule_work(&ctx->put_work);

[Severity: High]
Is it safe to unconditionally schedule ctx->put_work here?

If a DSP response arrives concurrently and the async worker has already started
executing fastrpc_context_put_wq(), its WORK_STRUCT_PENDING flag would be
clear. 

Could this allow the work to be queued a second time, triggering a
use-after-free in the workqueue core once the first execution frees the
context?

> +	}
>  	spin_unlock(&user->lock);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701-master-v2-1-566bf8b7bd16@oss.qualcomm.com?part=1