Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Critical] misc: fastrpc: unconditional list_del in error path races with concurrent unmap --
commit 03236356a22d5b8fc33413026fac2ff46d3f8883 Author: Yao Yiqi <[email protected]> misc: fastrpc: Fix double free in fastrpc_req_mmap() error path This commit aims to fix a double free and use-after-free in the fastrpc_req_mmap() error path. It modifies the error handling when copy_to_user() fails to remove the buffer from the mmaps list before unmapping, matching patterns introduced in prior patches. Cc: [email protected] > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index af18ff1992eed..436e046ed1cc0 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -2129,6 +2129,9 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, > char __user *argp) > > if (copy_to_user((void __user *)argp, &req, sizeof(req))) { > err = -EFAULT; > + spin_lock(&fl->lock); > + list_del(&buf->node); > + spin_unlock(&fl->lock); [Severity: Critical] Does this unconditionally remove the buffer from the list even if a concurrent thread has already removed it? If copy_to_user() blocks or is delayed (for example, via userfaultfd), a concurrent thread could call FASTRPC_IOCTL_MUNMAP, predicting or reading the partially copied address. This thread can extract the buffer from the list and free it while the first thread is still suspended: drivers/misc/fastrpc.c:fastrpc_req_munmap() { ... list_del(&iter->node); buf = iter; break; ... } If the suspended copy_to_user() then fails, the first thread will resume and execute this newly added list_del(&buf->node). Because the concurrent unmap already unlinked the buffer, its list pointers will have been set to LIST_POISON1 and LIST_POISON2. Could this trigger an immediate kernel oops due to a poisoned pointer dereference? > goto err_assign; > } > > @@ -2138,7 +2141,11 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, > char __user *argp) > return 0; > > err_assign: > - fastrpc_req_munmap_impl(fl, buf); > + if (fastrpc_req_munmap_impl(fl, buf)) { [Severity: Critical] Following the race sequence described above, if the concurrent unmap thread has already freed this buffer, does passing it to fastrpc_req_munmap_impl() here lead to a use-after-free and a subsequent double free? > + 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=1
