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

Re: [PATCH v2] misc: fastrpc: Fix race and 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:
- [High] misc: fastrpc: memory leak in fastrpc_req_mmap error path
--

commit 9824ed05459dfdc017a5b781dcfa0dc08576a6f0
Author: Yao Yiqi <yaoyiqi3@huawei.com>

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

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