Re: [PATCH v3 2/2] misc: fastrpc: don't publish fd before copy_to_user() succeeds

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The `fastrpc_user` context can be used-after-free if an exported dmabuf outlives the fastrpc device file descriptor.
- [Critical] Concurrent calls to `FASTRPC_IOCTL_MUNMAP` lead to list corruption, Use-After-Free, and Double-Free of a `fastrpc_buf`.
- [Critical] Concurrent calls to `FASTRPC_IOCTL_MEM_UNMAP` cause a Use-After-Free and Double-Free of a `fastrpc_map`.
--

--- Patch [2]: [PATCH v3 2/2] misc: fastrpc: don't publish fd before copy_to_user() succeeds ---
commit 385ee7a3198521d1682f7120112f746dede7b20d
Author: Baineng Shou <shoubaineng@gmail.com>
misc: fastrpc: don't publish fd before copy_to_user() succeeds

This commit fixes an issue where the DMA buffer file descriptor is published 
to userspace before a successful usercopy. It reserves an unused fd, performs 
the copy, and only installs the fd if the copy succeeds. On failure, it 
cleanly unwinds the fd and buffer allocations.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index f3a49384586d1..c5143cd257674 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
[ ... ]
> @@ -1709,24 +1709,20 @@ static int fastrpc_dmabuf_alloc(struct fastrpc_user *fl, char __user *argp)
>  		return err;
>  	}
>  
> -	bp.fd = dma_buf_fd(buf->dmabuf, O_ACCMODE);
> +	bp.fd = get_unused_fd_flags(O_ACCMODE);

[Severity: Critical]
This is a pre-existing issue, but does fastrpc_buf_alloc() fail to take a 
reference on the fastrpc_user context?

Looking just above this diff in fastrpc_dmabuf_alloc():

err = fastrpc_buf_alloc(fl, fl->sctx->dev, bp.size, &buf);

This allocates the buffer and assigns the user context to buf->fl, but does 
not increment the user context's reference count. If the fastrpc device file 
descriptor is closed, fastrpc_device_release() drops the reference to fl and 
frees it, meaning the exported dmabuf can outlive it.

If the dmabuf is closed later, fastrpc_release() calls fastrpc_buf_free(), 
which dereferences the freed context here:

fastrpc_ipa_to_dma_addr(buf->fl->cctx, buf->dma_addr)

Can this lead to a use-after-free of the fastrpc_user context?

[Severity: Critical]
This is also a pre-existing issue, but does concurrent execution of 
FASTRPC_IOCTL_MUNMAP lead to list corruption or double-frees?

In fastrpc_req_munmap(), the buf pointer is located under the lock:

buf = iter;
break;

The lock is then dropped without taking a reference count on the buffer. The 
raw pointer is passed to fastrpc_req_munmap_impl(). If two threads invoke 
the ioctl concurrently with the same arguments, could they both find the 
same buf and race? 

They would both concurrently execute this in fastrpc_req_munmap_impl():

list_del(&buf->node);
spin_unlock(&fl->lock);
fastrpc_buf_free(buf);

Does this result in list_del() and fastrpc_buf_free() executing twice on 
the same pointer?

[Severity: Critical]
This is another pre-existing issue, but does fastrpc_req_mem_unmap_impl() 
have a similar race condition?

The map is extracted from the list under the lock, and then the lock is 
dropped without incrementing the map's reference count:

map = iter;
break;
}
}
spin_unlock(&fl->lock);

Multiple threads could extract the same map pointer. If both threads 
successfully proceed, they will both call:

fastrpc_map_put(map);

Since fastrpc_map_get() was never called during the lookup, does the first 
thread drop the reference count to 0 and free the map prematurely? This 
could cause the second thread to trigger a use-after-free on failure paths 
or a double-free when calling fastrpc_map_put() again.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714114654.3885457-1-shoubaineng@gmail.com?part=2