Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] misc: fastrpc: reference cycle between fastrpc_user and dma_buf --
commit dec722bc667a70cb2ae899023a7e5c5a5ba8d6fb Author: Loic Poulain <[email protected]> misc: fastrpc: fix use-after-free of fastrpc_user in dma_buf release This commit fixes a use-after-free of struct fastrpc_user during dma_buf release by taking a reference on fl in fastrpc_dmabuf_alloc() and dropping it in fastrpc_release(). > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index af18ff1992eed..36722ed66edff 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -796,8 +796,10 @@ static void fastrpc_unmap_dma_buf(struct > dma_buf_attachment *attach, > static void fastrpc_release(struct dma_buf *dmabuf) > { > struct fastrpc_buf *buffer = dmabuf->priv; > + struct fastrpc_user *fl = buffer->fl; > > fastrpc_buf_free(buffer); > + fastrpc_user_put(fl); > } > > static int fastrpc_dma_buf_attach(struct dma_buf *dmabuf, > @@ -1802,10 +1804,12 @@ static int fastrpc_dmabuf_alloc(struct fastrpc_user > *fl, char __user *argp) > exp_info.size = bp.size; > exp_info.flags = O_RDWR; > exp_info.priv = buf; > + fastrpc_user_get(fl); > buf->dmabuf = dma_buf_export(&exp_info); > if (IS_ERR(buf->dmabuf)) { > err = PTR_ERR(buf->dmabuf); > fastrpc_buf_free(buf); > + fastrpc_user_put(fl); > return err; > } [Severity: High] Does taking a reference on fl for the lifetime of the exported dma_buf create a circular reference that leaks fl, map, and the coherent DMA buffer when a dma_buf allocated by fastrpc_dmabuf_alloc() is mapped into the same fastrpc_user session? When userspace maps an exported dma_buf into the session (for example via fastrpc_init_create_process() -> fastrpc_map_create() -> fastrpc_map_attach(), fastrpc_req_mem_map() -> fastrpc_map_create(), or fastrpc_internal_invoke() -> fastrpc_get_args() -> fastrpc_create_maps()), fastrpc_map_attach() acquires a reference on the dma_buf and places the map on fl->maps: drivers/misc/fastrpc.c:fastrpc_map_attach() { ... map->fl = fl; map->fd = fd; map->buf = dma_buf_get(fd); ... spin_lock(&fl->lock); list_add_tail(&map->node, &fl->maps); spin_unlock(&fl->lock); } In paths such as fastrpc_init_create_process() (or when a process exits without calling fastrpc_req_mem_unmap()), that map remains on fl->maps until the session is torn down. However, fastrpc_device_release() only frees fl->sctx and drops the file's reference on fl without draining fl->maps: drivers/misc/fastrpc.c:fastrpc_device_release() { ... fastrpc_session_free(cctx, fl->sctx); file->private_data = NULL; /* Release the reference taken in fastrpc_device_open */ fastrpc_user_put(fl); return 0; } Because fl->maps is only drained in fastrpc_user_free() when fl->refcount reaches zero: drivers/misc/fastrpc.c:fastrpc_user_free() { ... list_for_each_entry_safe(map, m, &fl->maps, node) fastrpc_map_put(map); } and fastrpc_free_map() is what calls dma_buf_put(map->buf) to trigger fastrpc_release(), fl->refcount cannot reach zero until fastrpc_release() calls fastrpc_user_put(fl), while fastrpc_release() cannot run until fastrpc_user_free() drops the map on fl->maps. Additionally, even if the dma_buf is not in fl->maps (for instance, when it is held open by another process after the fastrpc device fd is closed), keeping fl alive until fastrpc_release() delays fastrpc_user_free() past fastrpc_device_release(), leaving fl->init_mem, fl->maps, and fl->mmaps allocated in the SMMU context bank after fastrpc_session_free() has already released fl->sctx for reuse by another client. Since fastrpc_buf_free(), fastrpc_dma_buf_attach(), and fastrpc_mmap() only dereference buf->fl->cctx for fastrpc_ipa_to_dma_addr(), could struct fastrpc_buf store a pointer (and reference, if needed) to struct fastrpc_channel_ctx instead of pinning struct fastrpc_user? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
