Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Routing the qcom_scm_assign_mem() failure path to get_err introduces a massive DMA-buf and memory leak because fastrpc_free_map() aborts early on cleanup. Pre-existing issues: - [Critical] Lockless and un-referenced use of fastrpc_map pointers leads to Use-After-Free vulnerabilities. -- --- Patch [1]: [PATCH v2] misc: fastrpc: fix double-free in fastrpc_map_attach() error path --- commit e44cb66ae041e0f79b6c9be086dbcbf912aa8a9e Author: Yifei Gao <[email protected]> misc: fastrpc: fix double-free in fastrpc_map_attach() error path This patch attempts to fix a double-free in the error path of fastrpc_map_attach() by routing map->table-is-set failure branches to get_err instead of map_err. This makes fastrpc_free_map() the sole owner of the unmap/detach/put sequence for these branches. > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index eb6c2a78d3c7b..d480a87752a7f 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -881,7 +881,7 @@ static int fastrpc_map_attach(struct fastrpc_user *fl, > int fd, > dev_dbg(sess->dev, "Bad size passed len 0x%llx map size > 0x%llx\n", > len, map->size); > err = -EINVAL; > - goto map_err; > + goto get_err; > } > map->va = sg_virt(map->table->sgl); > map->len = len; [ ... ] > @@ -904,7 +904,7 @@ static int fastrpc_map_attach(struct fastrpc_user *fl, > int fd, > dev_err(sess->dev, > "Failed to assign memory with dma_addr %pad > size 0x%llx err %d\n", > &map->dma_addr, map->len, err); > - goto map_err; > + goto get_err; [Severity: High] Does routing this qcom_scm_assign_mem() failure to get_err introduce a memory and DMA-buf leak? When the forward assignment to TrustZone fails, map->attr is already set to FASTRPC_ATTR_SECUREMAP. If we jump to get_err, fastrpc_map_put() is called which in turn calls fastrpc_free_map(). Inside fastrpc_free_map(), it will see this flag and attempt a reverse qcom_scm_assign_mem(). Since the forward assignment just failed, the reverse assignment will also fail. Looking at fastrpc_free_map(), if the reverse assignment fails, it returns early: fastrpc_free_map() { ... if (map->attr & FASTRPC_ATTR_SECUREMAP) { ... err = qcom_scm_assign_mem(...); if (err) return; } ... } Will this early return completely skip dma_buf_unmap_attachment_unlocked(), dma_buf_detach(), dma_buf_put(), and kfree(map), leaving untrusted user space able to exhaust kernel memory? > } > } > spin_lock(&fl->lock); [Severity: Critical] This is a pre-existing issue, but can the lockless use of fastrpc_map pointers lead to a use-after-free elsewhere in this file? In fastrpc_req_mem_unmap_impl(), the fl->maps list is searched under fl->lock to find a map pointer. If the lock is then dropped without incrementing the object's reference count, concurrent ioctls could free the map while it is still being accessed. A similar pattern appears in fastrpc_put_args() where fastrpc_map_lookup() is called without taking a reference, returning an unreferenced pointer that is later passed to fastrpc_map_put(), causing a refcount imbalance. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
