Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [Critical] The `fastrpc_user` context can be used-after-free if an exported dmabuf outlives the fastrpc device file descriptor. - [Critical] Concurrent calls to `FASTRPC_IOCTL_MUNMAP` lead to list corruption, Use-After-Free, and Double-Free of a `fastrpc_buf`. - [Critical] Concurrent calls to `FASTRPC_IOCTL_MEM_UNMAP` cause a Use-After-Free and Double-Free of a `fastrpc_map`. -- --- Patch [2]: [PATCH v3 2/2] misc: fastrpc: don't publish fd before copy_to_user() succeeds --- commit 385ee7a3198521d1682f7120112f746dede7b20d Author: Baineng Shou <[email protected]> misc: fastrpc: don't publish fd before copy_to_user() succeeds This commit fixes an issue where the DMA buffer file descriptor is published to userspace before a successful usercopy. It reserves an unused fd, performs the copy, and only installs the fd if the copy succeeds. On failure, it cleanly unwinds the fd and buffer allocations. > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index f3a49384586d1..c5143cd257674 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c [ ... ] > @@ -1709,24 +1709,20 @@ static int fastrpc_dmabuf_alloc(struct fastrpc_user > *fl, char __user *argp) > return err; > } > > - bp.fd = dma_buf_fd(buf->dmabuf, O_ACCMODE); > + bp.fd = get_unused_fd_flags(O_ACCMODE); [Severity: Critical] This is a pre-existing issue, but does fastrpc_buf_alloc() fail to take a reference on the fastrpc_user context? Looking just above this diff in fastrpc_dmabuf_alloc(): err = fastrpc_buf_alloc(fl, fl->sctx->dev, bp.size, &buf); This allocates the buffer and assigns the user context to buf->fl, but does not increment the user context's reference count. If the fastrpc device file descriptor is closed, fastrpc_device_release() drops the reference to fl and frees it, meaning the exported dmabuf can outlive it. If the dmabuf is closed later, fastrpc_release() calls fastrpc_buf_free(), which dereferences the freed context here: fastrpc_ipa_to_dma_addr(buf->fl->cctx, buf->dma_addr) Can this lead to a use-after-free of the fastrpc_user context? [Severity: Critical] This is also a pre-existing issue, but does concurrent execution of FASTRPC_IOCTL_MUNMAP lead to list corruption or double-frees? In fastrpc_req_munmap(), the buf pointer is located under the lock: buf = iter; break; The lock is then dropped without taking a reference count on the buffer. The raw pointer is passed to fastrpc_req_munmap_impl(). If two threads invoke the ioctl concurrently with the same arguments, could they both find the same buf and race? They would both concurrently execute this in fastrpc_req_munmap_impl(): list_del(&buf->node); spin_unlock(&fl->lock); fastrpc_buf_free(buf); Does this result in list_del() and fastrpc_buf_free() executing twice on the same pointer? [Severity: Critical] This is another pre-existing issue, but does fastrpc_req_mem_unmap_impl() have a similar race condition? The map is extracted from the list under the lock, and then the lock is dropped without incrementing the map's reference count: map = iter; break; } } spin_unlock(&fl->lock); Multiple threads could extract the same map pointer. If both threads successfully proceed, they will both call: fastrpc_map_put(map); Since fastrpc_map_get() was never called during the lookup, does the first thread drop the reference count to 0 and free the map prematurely? This could cause the second thread to trigger a use-after-free on failure paths or a double-free when calling fastrpc_map_put() again. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
