Re: [PATCH] misc: fastrpc: Fix double free in fastrpc_req_mmap() error path

Re: [PATCH] misc: fastrpc: Fix double free in fastrpc_req_mmap() error path

[sashiko-bot]

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 <yaoyiqi3@huawei.com>

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: stable@vger.kernel.org

> 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/20260930032022.2957082-1-yaoyiqi3@huawei.com?part=1