Claude review: misc: fastrpc: fix UAF and kernel panic during cleanup on process abort

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Critical: Won't compile — `buf->phys` and `FASTRPC_PHYS` don't exist**

The `fastrpc_buf` struct (line 192) has a field `dma_addr` of type `dma_addr_t`, but the patch introduces references to a nonexistent `buf->phys` field and an undefined `FASTRPC_PHYS` macro:

In `__fastrpc_buf_alloc`:
```c
buf->virt = 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 code 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 appears the patch was developed against an out-of-tree or vendor kernel with a different `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` allocation 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_BIDIRECTIONAL);
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) putting the dma_buf reference (`dma_buf_put`), (2) removing the map from the user's list, or (3) freeing the map struct via `kfree(map)`. These happen later in the function at lines 376–383, but the early return skips them. This will leak both the `fastrpc_map` and hold dma_buf references indefinitely. The early return should jump past the DMA operations but still execute 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 = 0; i < FASTRPC_MAX_SESSIONS; i++) {
    if (cctx->session[i].sid == sess->sid) {
        spin_unlock_irqrestore(&cctx->lock, flags);
        mutex_lock(&cctx->session[i].mutex);
        cctx->session[i].dev = NULL;
        mutex_unlock(&cctx->session[i].mutex);
        spin_lock_irqsave(&cctx->lock, flags);
        cctx->session[i].valid = false;
        cctx->sesscount--;
    }
}
spin_unlock_irqrestore(&cctx->lock, flags);
```

Dropping the spinlock mid-loop means another thread could modify the session array while the loop is iterating. Since the loop checks `sid` to identify matching sessions, and other code could change session state while the lock is dropped, this introduces a TOCTOU window. Additionally, `sesscount` is modified without consistent lock protection (decremented under the spinlock but after the mutex-protected `dev = 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 individually.

**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 other session initialization.

**Issue: `memcpy` of session copies the mutex**

```c
dup_sess = &cctx->session[cctx->sesscount++];
memcpy(dup_sess, sess, sizeof(*dup_sess));
mutex_init(&dup_sess->mutex);
dup_sess->allocated = true;
```

The `memcpy` copies the entire `fastrpc_session_ctx` including the now-embedded mutex, then immediately re-initializes it. While the `mutex_init` overwrites 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 individually, 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`, `FASTRPC_PHYS`), memory/resource leaks (buf and map not freed when device is gone), and questionable locking patterns. It needs significant rework before it's suitable for merging.

---
Generated by Claude Code Patch Reviewer