Re: [PATCH v3] 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:
- [High] fastrpc: missing 'sent' flag causes indefinite hang and UAF on retry
- [High] fastrpc: resumed context uses stale user-space pointers for DSP output
- [High] fastrpc: Use-After-Free in fastrpc_rpmsg_callback due to concurrent teardown
- [Medium] fastrpc: memory leak of timed-out contexts on channel teardown
--

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

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

fastrpc invokes work by sending an RPC message to the DSP and blocking
in wait_for_completion_interruptible() until the DSP responds. If a
signal arrives during this wait, the syscall returns -ERESTARTSYS and
the invoke context which holds the in-flight DMA buffers and
completion state is left stranded in fl->pending.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 5ac7b3e78ba7e..f51cb772e6831 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
[ ... ]
> @@ -1370,9 +1444,21 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel,
>  		return -EPERM;
>  	}
>  
> -	ctx = fastrpc_context_alloc(fl, kernel, sc, args);
> -	if (IS_ERR(ctx))
> -		return PTR_ERR(ctx);
> +	if (!kernel)
> +		ctx = fastrpc_context_restore_interrupted(fl, handle, sc);
> +
> +	if (ctx) {
> +		/* ctx->args is already populated from before the interruption. */
> +		resumed = true;
> +	} else {
> +		ctx = fastrpc_context_alloc(fl, kernel, sc, args);
> +		if (IS_ERR(ctx))
> +			return PTR_ERR(ctx);
> +		ctx->handle = handle;
> +	}
> +

[Severity: High]
Does this code ignore the new args structure provided by the retrying user-space
caller?

The code appears to retain the stale pointers captured before the interruption,
failing to update ctx->args with the retry's arguments. When the DSP responds,
fastrpc_put_args() might copy the output to the original, potentially freed or
repurposed user-space addresses.

> +	if (resumed)
> +		goto wait;

[Severity: High]
Can skipping the message sending cause a thread to get permanently hung?

Because the missing 'sent' flag check fails to detect that the message wasn't
delivered initially, the resumed context may incorrectly skip sending the
message here.

>  
>  	err = fastrpc_get_args(kernel, ctx);
>  	if (err)
[ ... ]
> @@ -1409,21 +1496,13 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel,
>  		goto bail;
>  
>  bail:
> -	if (err != -ERESTARTSYS && err != -ETIMEDOUT) {
> -		/* We are done with this compute context */
> -		spin_lock(&fl->lock);
> -		list_del(&ctx->node);
> -		spin_unlock(&fl->lock);
> -		fastrpc_context_put(ctx);
> -	}
> -
>  	if (err == -ERESTARTSYS) {
> +		fastrpc_context_save_interrupted(ctx);
> +	} else {

[Severity: High]
Is the context unconditionally parked as resumable without verifying if the
message was actually sent?

The commit message mentions this is handled via a 'sent' flag, but the flag
appears to be missing in the patch, which could park unsent contexts as
resumable.

>  		spin_lock(&fl->lock);
> -		list_for_each_entry_safe(buf, b, &fl->mmaps, node) {
> -			list_del(&buf->node);
> -			list_add_tail(&buf->node, &fl->cctx->invoke_interrupted_mmaps);
> -		}
> +		list_del_init(&ctx->node);
>  		spin_unlock(&fl->lock);
> +		fastrpc_context_put(ctx);
>  	}

[Severity: Medium]
When a context times out (err == -ETIMEDOUT), does this code remove it from the
tracking lists?

This would make it unreachable for the fastrpc_notify_users() cleanup sweep,
potentially leaking the context and its DSP reference if the DSP never responds
and the channel is torn down.

[ ... ]
> @@ -2669,13 +2762,22 @@ static void fastrpc_notify_users(struct fastrpc_user *user)
>  		ctx->retval = -EPIPE;
>  		complete(&ctx->work);
>  	}
> +
> +	/* Drops only the worker ref; the invoker ref is released by fastrpc_user_free(). */
> +	list_for_each_entry(ctx, &user->interrupted, node) {

[Severity: Medium]
Because the timed-out contexts are removed from fl->pending and fl->interrupted,
does this teardown routine miss cleaning them up when the DSP reference is still
held?

> +		ctx->retval = -EPIPE;
> +		complete(&ctx->work);
> +		if (!ctx->put_work_scheduled) {
> +			ctx->put_work_scheduled = true;
> +			schedule_work(&ctx->put_work);
> +		}
> +	}
>  	spin_unlock(&user->lock);
>  }
[ ... ]
> @@ -2778,9 +2877,11 @@ static int fastrpc_rpmsg_callback(struct rpmsg_device *rpdev, void *data,
>  	/*
>  	 * The DMA buffer associated with the context cannot be freed in
>  	 * interrupt context so schedule it through a worker thread to
> -	 * avoid a kernel BUG.
> +	 * avoid a kernel BUG. fastrpc_context_schedule_put_work() guards
> +	 * against a concurrent fastrpc_notify_users() also scheduling this
> +	 * same work item for an interrupted context.
>  	 */
> -	schedule_work(&ctx->put_work);
> +	fastrpc_context_schedule_put_work(ctx);
>  
>  	return 0;
>  }

[Severity: High]
Can a concurrent channel teardown or file descriptor closure cause a
use-after-free here?

Earlier in this function, the context is found in the IDR and the cctx->lock is
dropped without elevating the refcount. Concurrent teardown paths can drop both
the invoker and DSP references, freeing the context before the callback reads
ctx->ctxid or accesses it here.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002-anandu-fastrpc-interrupted-invoke-v3-v3-1-31e90439d2ba@oss.qualcomm.com?part=1