From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: misc: fastrpc: fix use-after-free of fastrpc_user in workqueue context Date: Thu, 30 Apr 2026 10:52:17 +1000 Message-ID: In-Reply-To: <20260427074021.3774769-1-anandu.e@oss.qualcomm.com> References: <20260427074021.3774769-1-anandu.e@oss.qualcomm.com> <20260427074021.3774769-1-anandu.e@oss.qualcomm.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Commit message**: Well-written. Clearly establishes the race, identifies = all three UAF dereference sites, explains the fix approach, and includes th= e crash trace. Good use of Fixes/Cc: stable tags. **kref_init ordering (bug)** In `fastrpc_device_open()`, the kref is initialized **after** the user is a= dded to the channel's user list: ```c spin_lock_irqsave(&cctx->lock, flags); list_add_tail(&fl->user, &cctx->users); spin_unlock_irqrestore(&cctx->lock, flags); + kref_init(&fl->refcount); ``` The refcount must be initialized **before** the structure becomes visible t= o other code paths. `fastrpc_rpmsg_remove()` traverses `cctx->users` under = the same lock, and could race with open. Move `kref_init(&fl->refcount)` be= fore the `list_add_tail`, ideally next to `spin_lock_init`/`mutex_init` whe= re other initialization happens. **Use-after-free on fl->pending in fastrpc_user_free() (bug)** This is the most significant issue. The patch moves the pending context cle= anup from `fastrpc_device_release()` into the kref release callback `fastrp= c_user_free()`: ```c +static void fastrpc_user_free(struct kref *ref) +{ + ... + list_for_each_entry_safe(ctx, n, &fl->pending, node) { + list_del(&ctx->node); + fastrpc_context_put(ctx); + } ``` But `fastrpc_context_free()` (the context kref release) does `kfree(ctx)` *= *without** removing `ctx->node` from `fl->pending`, then calls `fastrpc_use= r_put()`: ```c static void fastrpc_context_free(struct kref *ref) { ... kfree(ctx); =20 fastrpc_channel_ctx_put(cctx); + /* Release the reference taken in fastrpc_context_alloc() */ + fastrpc_user_put(fl); } ``` Consider the "abandoned invoke" scenario (invoker interrupted with `-ERESTA= RTSYS` or timed out with `-ETIMEDOUT`): 1. Invoker sets `is_work_done =3D true` (or equivalent), does **not** call = `list_del(&ctx->node)` or `fastrpc_context_put(ctx)`. Context remains on `f= l->pending` with refcount 1; user refcount is 2 (initial + context). 2. `fastrpc_device_release()` calls `fastrpc_user_put()` =E2=86=92 user ref= count drops to 1. 3. DSP eventually responds; callback schedules `ctx->put_work`. 4. Workqueue runs `fastrpc_context_put_wq()` =E2=86=92 `fastrpc_context_fre= e()`: - `kfree(ctx)` =E2=80=94 context memory freed, but `ctx->node` is **stil= l linked** into `fl->pending`. - `fastrpc_user_put(fl)` =E2=86=92 user refcount drops to 0 =E2=86=92 `f= astrpc_user_free()` runs. 5. `fastrpc_user_free()` does `list_for_each_entry_safe` over `fl->pending`= =E2=80=94 dereferences the just-freed `ctx`. **Use-after-free.** With the patch's refcounting model, every context on `fl->pending` holds a = user reference. For `fastrpc_user_free()` to execute, **all** those context= s must have already been freed (their `fastrpc_context_free()` called `fast= rpc_user_put()`). So the pending list iteration in `fastrpc_user_free()` ca= n never find a live context =E2=80=94 it either finds an empty list or free= d entries. To fix this, `fastrpc_context_free()` must remove `ctx->node` from `fl->pen= ding` before `kfree(ctx)`. Since the normal invoke path (`fastrpc_internal_= invoke` bail label) already does `list_del(&ctx->node)`, you'd need to avoi= d double-delete. A clean approach: - Change `fastrpc_internal_invoke`'s `list_del(&ctx->node)` to `list_del_in= it(&ctx->node)`. - In `fastrpc_context_free()`, before `kfree(ctx)`, add: ```c spin_lock(&fl->lock); if (!list_empty(&ctx->node)) list_del(&ctx->node); spin_unlock(&fl->lock); ``` - Then the `fl->pending` iteration in `fastrpc_user_free()` becomes a safet= y net for any contexts that somehow weren't cleaned up, rather than a guara= nteed UAF. **fastrpc_session_free ordering (minor observation)** `fastrpc_session_free(cctx, fl->sctx)` remains in `fastrpc_device_release()= ` rather than moving to `fastrpc_user_free()`. This means if the user struc= ture outlives `device_release` (because a context still holds a reference),= the session is already freed when the deferred cleanup runs. This appears = safe since `fastrpc_buf_free()` and `fastrpc_free_map()` dereference `fl->c= ctx`, not `fl->sctx`, but it deserves an explicit comment noting the intent= ional ordering. **Trivial comment nit** ```c + /* Reference count */ + struct kref refcount; ``` This comment adds no information beyond what `struct kref refcount` already= conveys. Either drop it or say something about the lifetime contract (e.g.= , `/* one ref per open + one per in-flight ctx */`). **Forward declaration** ```c +static void fastrpc_context_put(struct fastrpc_invoke_ctx *ctx); ``` Needed because `fastrpc_user_free()` is defined before `fastrpc_context_put= ()`. This is acceptable but worth noting: if the pending list iteration is = removed from `fastrpc_user_free()` (because it's unreachable with valid dat= a), this forward declaration can also be removed. --- Generated by Claude Code Patch Reviewer