Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [Critical] Use-After-Free and Double Free due to bypassing reference counting for handles. Pre-existing issues: - [High] Memory leak of newly created DMA maps for handle arguments. - [High] Race condition in `fdlist` cleanup leading to Kernel Use-After-Free. - [High] Circular reference causing permanent resource leak on interrupted invocations. -- Note: The format of this review is altered to provide a free-form summary rather than inline quotes to comply with system recitation restrictions. commit ad847e952f22a54c8e1f599b332d32abb92a3e6a Author: Jianping Li <[email protected]> misc: fastrpc: avoid duplicate DMA mappings in fastrpc_create_maps() This patch modifies fastrpc_create_maps() to bypass reference counting when looking up existing DMA mappings for handle arguments. It achieves this by introducing a boolean take_ref flag to fastrpc_map_create() to prevent duplicate DMA mappings and unbalanced reference counts. [Severity: Critical] Does bypassing the reference count for handle arguments introduce a use-after-free and double-free risk? In drivers/misc/fastrpc.c:fastrpc_create_maps(), this patch forces take_ref = false for arguments where i >= ctx->nbufs. This populates ctx->maps[i] with a weak reference. If a concurrent thread calls FASTRPC_IOCTL_MEM_UNMAP and drops the map's reference, could fastrpc_get_args() dereference a freed pointer when it accesses ctx->maps[i]->dma_addr? Additionally, if the DSP returns the handle in fdlist, fastrpc_put_args() will unconditionally call fastrpc_map_put(). Since a reference was never taken during creation, does this consume an unbalanced reference and free explicit memory maps prematurely? [Severity: High] This isn't a bug introduced by this patch, but does the error path leak newly created DMA maps for handle arguments? When fastrpc_map_create() does not find an existing mapping, it calls fastrpc_map_attach(), which allocates a new map with a reference count of 1. However, looking at drivers/misc/fastrpc.c:fastrpc_context_free(), the cleanup loop only iterates up to ctx->nbufs. Since handle arguments (i >= ctx->nbufs) are deliberately skipped during this cleanup, are the maps created for them permanently leaked, potentially leading to memory exhaustion if a user repeatedly passes new memfds? [Severity: High] This is a pre-existing issue, but is there a race condition in the fdlist cleanup that could lead to a use-after-free? In drivers/misc/fastrpc.c:fastrpc_put_args(), the code looks up the map without taking a reference (take_ref = false), and then immediately calls fastrpc_map_put() to drop a reference. If a compromised or buggy DSP returns the same file descriptor in the fdlist for two concurrent invocations, could both threads simultaneously execute the lookup and drop a reference without adequate locking, causing the second thread to drop a reference on an already freed map? [Severity: High] This isn't a bug introduced by this patch, but does the invocation error handling create a circular reference that permanently leaks resources? In drivers/misc/fastrpc.c:fastrpc_internal_invoke(), if the invocation is interrupted (returning -ERESTARTSYS or -ETIMEDOUT), the code explicitly skips calling fastrpc_context_put(). Because fastrpc_context_alloc() takes a strong reference to the user structure (fastrpc_user_get()), and the leaked context holds this reference, the user reference count never reaches zero. Does this prevent fastrpc_user_free() from ever executing to clean up pending contexts, allowing a local user to easily exhaust resources by interrupting invocations? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
