Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
Pre-existing issues: - [Critical] Integer overflow in `fastrpc_get_payload_size` leads to a massive kernel heap buffer overflow during metadata initialization. - [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition and lockless access to `cctx->rpdev` can lead to NULL pointer dereferences when `fastrpc_rpmsg_remove()` runs concurrently. - [High] Exported DMA buffers retain a pointer to the `fastrpc_user` object (`buf->fl`) without holding a reference, causing a use-after-free if the fastrpc device is closed before the DMA buffer. - [High] Missing locking and reference counting in `fastrpc_req_munmap` leads to concurrent use-after-free and double-free vulnerabilities. - [High] Missing reference counting during list traversal in `fastrpc_req_mem_unmap_impl` leads to use-after-free. - [High] The kernel blindly trusts the untrusted DSP to supply a list of FDs for unmapping, allowing a malicious DSP payload to force use-after-free conditions. -- --- Patch [1]: [PATCH] misc: fastrpc: drop channel context ref on failed open --- commit f86e434afd2dbbcb5bb07286bb0b83dad48769a0 Author: Xu Rao <[email protected]> misc: fastrpc: drop channel context ref on failed open This patch drops the channel-context reference on the failed-open path before freeing the partially initialized fastrpc_user. This prevents permanently pinning the channel context if session allocation fails. > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -1671,6 +1671,7 @@ static int fastrpc_device_open(struct inode *inode, > struct file *filp) > fl->sctx = fastrpc_session_alloc(fl); > if (!fl->sctx) { > dev_err(&cctx->rpdev->dev, "No session available\n"); > + fastrpc_channel_ctx_put(cctx); > mutex_destroy(&fl->mutex); > kfree(fl); [Severity: High] This isn't a bug introduced by this patch, but while reviewing this file I noticed a potential race condition. In fastrpc_internal_invoke(), the code performs a lockless check of rpdev: drivers/misc/fastrpc.c:fastrpc_internal_invoke() { ... if (!fl->cctx->rpdev) return -EPIPE; ... } Concurrently, if the device unbinds, fastrpc_rpmsg_remove() sets this to NULL: drivers/misc/fastrpc.c:fastrpc_rpmsg_remove() { ... cctx->rpdev = NULL; ... } The ioctl thread then unconditionally accesses it in fastrpc_invoke_send(): drivers/misc/fastrpc.c:fastrpc_invoke_send() { ... ret = rpmsg_send(cctx->rpdev->ept, (void *)msg, sizeof(*msg)); ... } Could this lead to a NULL pointer dereference if the device unbinds while an ioctl is in progress? [Severity: High] This is a pre-existing issue, but I have a question about DMA buffer allocation. When exporting a buffer, the allocation function sets the fl pointer but does not elevate the reference count: drivers/misc/fastrpc.c:fastrpc_dmabuf_alloc() { ... err = fastrpc_buf_alloc(fl, fl->sctx->dev, bp.size, &buf); ... } If the application closes the fastrpc device first, the fastrpc_user object is freed. Later, closing the DMA buffer triggers the cleanup callback which dereferences the freed object: drivers/misc/fastrpc.c:fastrpc_buf_free() { ... fastrpc_ipa_to_dma_addr(buf->fl->cctx, buf->dma_addr) ... } Does this cause a use-after-free if the device file descriptor is closed before the exported DMA buffer? [Severity: Critical] This isn't a bug introduced by this patch, but does the payload size calculation have an integer overflow? In fastrpc_get_payload_size(), lengths are accumulated: drivers/misc/fastrpc.c:fastrpc_get_payload_size() { ... size += (ctx->olaps[oix].mend - ctx->olaps[oix].mstart); ... } If a user provides large lengths, could this 64-bit sum wrap around to a small positive value? The undersized buffer would then be populated using the larger nscalars count: drivers/misc/fastrpc.c:fastrpc_get_args() { ... list = fastrpc_invoke_buf_start(rpra, ctx->nscalars); pages = fastrpc_phy_page_start(list, ctx->nscalars); ... } Can this lead to a heap buffer overflow during metadata initialization? [Severity: High] This is a pre-existing issue, but there appears to be a concurrency problem during unmapping. In fastrpc_req_munmap(), the lock is dropped without removing the buffer from the list or taking a reference: drivers/misc/fastrpc.c:fastrpc_req_munmap() { ... spin_unlock(&fl->lock); ... } The implementation then unconditionally deletes and frees the buffer: drivers/misc/fastrpc.c:fastrpc_req_munmap_impl() { ... list_del(&buf->node); spin_unlock(&fl->lock); fastrpc_buf_free(buf); ... } If two threads concurrently call the munmap ioctl with the same arguments, could they both find the same buffer and cause list corruption and a double-free? [Severity: High] This isn't a bug introduced by this patch, but a similar reference counting issue seems to exist in fastrpc_req_mem_unmap_impl(). After finding the map, the lock is dropped without elevating the reference count: drivers/misc/fastrpc.c:fastrpc_req_mem_unmap_impl() { ... spin_unlock(&fl->lock); ... } The code then accesses map fields and drops a reference it never acquired: drivers/misc/fastrpc.c:fastrpc_req_mem_unmap_impl() { ... req_msg.len = map->len; ... fastrpc_map_put(map); ... } Does this allow concurrent threads to drop the reference count multiple times, leading to a use-after-free? [Severity: High] This is a pre-existing issue, but does the kernel blindly trust file descriptors provided by the DSP? In fastrpc_put_args(), it reads an array of FDs from shared memory and arbitrarily drops their references: drivers/misc/fastrpc.c:fastrpc_put_args() { ... if (!fastrpc_map_lookup(fl, (int)fdlist[i], &mmap, false)) fastrpc_map_put(mmap); ... } Because the lookup is done without taking a reference, the put operation lowers the active refcount. Can a malicious DSP payload supply arbitrary active file descriptors and force the kernel to drop their references, causing a use-after-free for actively used maps? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
