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 UAF and kernel panic during cleanup on process abort Date: Tue, 28 Apr 2026 14:37:42 +1000 Message-ID: In-Reply-To: <20260427105310.4056-1-jianping.li@oss.qualcomm.com> References: <20260427105310.4056-1-jianping.li@oss.qualcomm.com> <20260427105310.4056-1-jianping.li@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 **Critical: Won't compile =E2=80=94 `buf->phys` and `FASTRPC_PHYS` don't ex= ist** The `fastrpc_buf` struct (line 192) has a field `dma_addr` of type `dma_add= r_t`, but the patch introduces references to a nonexistent `buf->phys` fiel= d and an undefined `FASTRPC_PHYS` macro: In `__fastrpc_buf_alloc`: ```c buf->virt =3D dma_alloc_coherent(dev, buf->size, (dma_addr_t *)&buf->phys, = GFP_KERNEL); ``` In `fastrpc_buf_free`: ```c dma_free_coherent(buf->dev, buf->size, buf->virt, FASTRPC_PHYS(buf->phys)); ``` Neither `buf->phys` nor `FASTRPC_PHYS` are defined anywhere. The original c= ode correctly uses `buf->dma_addr` and calls `fastrpc_ipa_to_dma_addr()` to= extract the SMMU physical address. This is a clear build failure. It appea= rs the patch was developed against an out-of-tree or vendor kernel with a d= ifferent `fastrpc_buf` definition. **Bug: Memory leak in `fastrpc_buf_free` when device is gone** ```c static void fastrpc_buf_free(struct fastrpc_buf *buf) { ... mutex_lock(&fl->sctx->mutex); if (fl->sctx->dev) { dma_free_coherent(buf->dev, buf->size, buf->virt, FASTRPC_PHYS(buf->phys)); kfree(buf); } mutex_unlock(&fl->sctx->mutex); } ``` When `fl->sctx->dev` is NULL, `kfree(buf)` is never called. The `buf` alloc= ation is leaked. The DMA memory may be reasonably left alone (the device is= gone), but the `struct fastrpc_buf` itself must still be freed. `kfree(buf= )` should be moved outside the `if` block. **Bug: Resource leak in `fastrpc_free_map` when device is gone** ```c mutex_lock(&fl->sctx->mutex); if (!fl->sctx->dev) { mutex_unlock(&fl->sctx->mutex); return; } dma_buf_unmap_attachment_unlocked(map->attach, map->table, DMA_BIDIRECTIONA= L); dma_buf_detach(map->buf, map->attach); dma_buf_put(map->buf); mutex_unlock(&fl->sctx->mutex); ``` When `fl->sctx->dev` is NULL, the function returns early without: (1) putti= ng the dma_buf reference (`dma_buf_put`), (2) removing the map from the use= r's list, or (3) freeing the map struct via `kfree(map)`. These happen late= r in the function at lines 376=E2=80=93383, but the early return skips them= . This will leak both the `fastrpc_map` and hold dma_buf references indefin= itely. The early return should jump past the DMA operations but still execu= te the list removal and kfree. **Issue: `fastrpc_cb_remove` drops and re-acquires spinlock in a loop** ```c spin_lock_irqsave(&cctx->lock, flags); for (i =3D 0; i < FASTRPC_MAX_SESSIONS; i++) { if (cctx->session[i].sid =3D=3D sess->sid) { spin_unlock_irqrestore(&cctx->lock, flags); mutex_lock(&cctx->session[i].mutex); cctx->session[i].dev =3D NULL; mutex_unlock(&cctx->session[i].mutex); spin_lock_irqsave(&cctx->lock, flags); cctx->session[i].valid =3D false; cctx->sesscount--; } } spin_unlock_irqrestore(&cctx->lock, flags); ``` Dropping the spinlock mid-loop means another thread could modify the sessio= n array while the loop is iterating. Since the loop checks `sid` to identif= y matching sessions, and other code could change session state while the lo= ck is dropped, this introduces a TOCTOU window. Additionally, `sesscount` i= s modified without consistent lock protection (decremented under the spinlo= ck but after the mutex-protected `dev =3D NULL` already occurred without it= ). This pattern is fragile. A cleaner approach would be to collect matching= session indices first, then release the spinlock and take each mutex indiv= idually. **Issue: `allocated` flag is redundant** The new `allocated` boolean tracks whether `mutex_init` was called so that = `mutex_destroy` can be called conditionally in `fastrpc_channel_ctx_free`. = However, `mutex_destroy` on an uninitialized (zero-filled) mutex is a no-op= in production kernels (it's only meaningful with `CONFIG_DEBUG_MUTEXES`). = Since `fastrpc_channel_ctx` is allocated with `kzalloc`, the sessions array= is zeroed. The `allocated` flag adds complexity without clear benefit. If = it's kept, it should at least be set under the spinlock consistently with o= ther session initialization. **Issue: `memcpy` of session copies the mutex** ```c dup_sess =3D &cctx->session[cctx->sesscount++]; memcpy(dup_sess, sess, sizeof(*dup_sess)); mutex_init(&dup_sess->mutex); dup_sess->allocated =3D true; ``` The `memcpy` copies the entire `fastrpc_session_ctx` including the now-embe= dded mutex, then immediately re-initializes it. While the `mutex_init` over= writes the copied mutex data, copying a live mutex is technically undefined= behavior and may trigger lockdep warnings with `CONFIG_DEBUG_MUTEXES`. The= initialization order should be explicit: copy the relevant fields individu= ally, or at minimum document that the mutex is intentionally re-initialized= after the copy. **Minor: `(dma_addr_t *)&buf->phys` cast** Even ignoring the wrong field name, casting an address to `(dma_addr_t *)` = is a type-punning concern if the underlying field isn't actually `dma_addr_= t`. The original code correctly passes `&buf->dma_addr` without a cast. **Summary**: The patch addresses a real problem (race between cleanup paths= and device teardown), but introduces compilation failures (`buf->phys`, `F= ASTRPC_PHYS`), memory/resource leaks (buf and map not freed when device is = gone), and questionable locking patterns. It needs significant rework befor= e it's suitable for merging. --- Generated by Claude Code Patch Reviewer