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 <[email protected]> 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/[email protected]?part=1
