Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Critical] The patch introduces a critical use-after-free and list corruption vulnerability in the fastrpc_req_mmap() error path. Pre-existing issues: - [High] Concurrent accesses to fl->mmaps without holding fl->lock when handling -ERESTARTSYS. -- commit a53abf279a47f6742d5cb44fb5a332592c4787b8 Author: Ekansh Gupta <[email protected]> misc: fastrpc: Remove buffer from list prior to unmap operation This commit removes the buffer from the fastrpc user's mapped list before calling the unmap operation to prevent concurrent unmap races. If the unmap operation fails, the buffer is re-added to the list. > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index 96961217b8569..517884000331b 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -1889,9 +1889,6 @@ static int fastrpc_req_munmap_impl(struct fastrpc_user > *fl, struct fastrpc_buf * > &args[0]); > if (!err) { > dev_dbg(dev, "unmmap\tpt 0x%09lx OK\n", buf->raddr); > - spin_lock(&fl->lock); > - list_del(&buf->node); > - spin_unlock(&fl->lock); [Severity: Critical] Does removing this list_del() introduce a use-after-free in the error path of fastrpc_req_mmap()? When fastrpc_req_mmap() encounters a copy_to_user() failure after successfully adding the buffer to the fl->mmaps list, it jumps to the err_assign label and calls fastrpc_req_munmap_impl() without first unlinking the buffer: drivers/misc/fastrpc.c:fastrpc_req_mmap() { ... 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; } ... err_assign: fastrpc_req_munmap_impl(fl, buf); return err; } Since fastrpc_req_munmap_impl() no longer removes the buffer from fl->mmaps before calling fastrpc_buf_free(), the freed buffer will remain in the list. Subsequent traverses of fl->mmaps will trigger a use-after-free. > fastrpc_buf_free(buf); > } else { > dev_err(dev, "unmmap\tpt 0x%09lx ERROR\n", buf->raddr); [ ... ] > @@ -1924,7 +1923,14 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, > char __user *argp) > return -EINVAL; > } > > - return fastrpc_req_munmap_impl(fl, buf); > + err = fastrpc_req_munmap_impl(fl, buf); [Severity: High] This isn't a bug introduced by this patch, but while reviewing the mmap list handling here, I noticed concurrent modifications to fl->mmaps happen locklessly in fastrpc_internal_invoke() when handling -ERESTARTSYS: drivers/misc/fastrpc.c:fastrpc_internal_invoke() { ... if (err == -ERESTARTSYS) { list_for_each_entry_safe(buf, b, &fl->mmaps, node) { list_del(&buf->node); list_add_tail(&buf->node, &fl->cctx->invoke_interrupted_mmaps); } } ... } Can this lead to list corruption if another thread concurrently executes fastrpc_req_mmap() or fastrpc_req_munmap() and modifies fl->mmaps with fl->lock held? > + if (err) { > + spin_lock(&fl->lock); > + list_add_tail(&buf->node, &fl->mmaps); > + spin_unlock(&fl->lock); > + } > + > + return err; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
