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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Exported DMA buffers maintain a raw pointer to `fastrpc_user` (`buf->fl`) without holding a reference, leading to a Use-After-Free if the DMA buffer outlives the fastrpc device descriptor.
--

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

misc: fastrpc: don't publish fd before copy_to_user() succeeds

This commit fixes an issue where a DMA buffer file descriptor is installed
into the fd table before successfully copying the fd number back to
userspace. It updates the allocation path to reserve the fd first, attempt
the user copy, and only publish the fd upon success to prevent leaking
access to the fd on failure.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> --- 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);
>  	if (bp.fd < 0) {
>  		dma_buf_put(buf->dmabuf);
> -		return -EINVAL;
> +		return bp.fd;
>  	}
>  
>  	if (copy_to_user(argp, &bp, sizeof(bp))) {
> -		/*
> -		 * The usercopy failed, but we can't do much about it, as
> -		 * dma_buf_fd() already called fd_install() and made the
> -		 * file descriptor accessible for the current process. It
> -		 * might already be closed and dmabuf no longer valid when
> -		 * we reach this point. Therefore "leak" the fd and rely on
> -		 * the process exit path to do any required cleanup.
> -		 */
> +		put_unused_fd(bp.fd);
> +		dma_buf_put(buf->dmabuf);
>  		return -EFAULT;
>  	}
>  
> +	dma_buf_fd_install(buf->dmabuf, bp.fd);

[Severity: Critical]
This is a pre-existing issue, but does exporting this dma-buf introduce a
use-after-free risk if the dma-buf fd outlives the fastrpc device fd?

When the fastrpc device fd is closed, the fastrpc_user struct (fl) is
released. However, the exported dma-buf maintains a raw pointer to it via
buf->fl without holding a reference.

If an unprivileged user allocates a dma-buf here, closes the fastrpc device
fd, and then calls close() or mmap() on the dma-buf fd, it triggers
dma_buf_put() or mmap(). Those paths call into fastrpc_buf_free() or
fastrpc_mmap(), which dereference the dangling buf->fl pointer:

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

Is there a missing reference count increment on fastrpc_user when exporting
the buffer, or should the dma-buf release operations handle the device
teardown differently?

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