Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] misc: fastrpc: memory leak in fastrpc_req_mmap error path --
commit 9824ed05459dfdc017a5b781dcfa0dc08576a6f0 Author: Yao Yiqi <[email protected]> misc: fastrpc: Fix race and double free in fastrpc_req_mmap() error path This commit defers adding the allocated buffer to fl->mmaps until after copy_to_user() succeeds. This prevents concurrent threads from accessing the buffer prematurely, fixing a race condition and a double free. Cc: [email protected] > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index d4fac2caca867..d2c09bffd0af4 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -2153,15 +2153,15 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, > char __user *argp) > } > } > > - spin_lock(&fl->lock); > - list_add_tail(&buf->node, &fl->mmaps); > - spin_unlock(&fl->lock); > - > if (copy_to_user((void __user *)argp, &req, sizeof(req))) { > err = -EFAULT; > goto err_assign; > } > > + spin_lock(&fl->lock); > + list_add_tail(&buf->node, &fl->mmaps); > + spin_unlock(&fl->lock); > + [Severity: High] Could this introduce a permanent DMA memory leak on the error path? fastrpc_req_mmap() is reachable via the FASTRPC_IOCTL_MMAP ioctl. If copy_to_user() or qcom_scm_assign_mem() fails (which is trivial to force from userspace by passing an invalid address), the error path jumps to err_assign and calls fastrpc_req_munmap_impl(fl, buf). If the IPC call to unmap the buffer from the DSP fails, fastrpc_req_munmap_impl() intentionally leaves the buffer unfreed to prevent reuse while the DSP still has access: drivers/misc/fastrpc.c:fastrpc_req_munmap_impl() { ... if (!err) { dev_dbg(dev, "unmmap\tpt 0x%09lx OK\n", buf->raddr); fastrpc_buf_free(buf); } else { dev_err(dev, "unmmap\tpt 0x%09lx ERROR\n", buf->raddr); } } Previously, since the buffer was already on fl->mmaps, it would eventually be cleaned up by fastrpc_user_free(). Because this patch delays adding the buffer to fl->mmaps until after copy_to_user() succeeds, the kernel loses its only reference to the allocation if the DSP unmap fails. Can we ensure the buffer is still tracked for cleanup in this scenario? > dev_dbg(dev, "mmap\t\tpt 0x%09lx OK [len 0x%08llx]\n", > buf->raddr, buf->size); > -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
