Re: [PATCH v7 3/4] drm/prime: use dma_buf_fd_install() to preserve export tracing

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

Pre-existing issues:
- [High] The file descriptor is published to userspace before copy_to_user() succeeds, violating the dma_buf_fd_install() API contract and causing a resource leak on error.
--

--- Patch [3]: [PATCH v7 3/4] drm/prime: use dma_buf_fd_install() to preserve export tracing ---
commit edfa3fdf8153dcc15c693c10359892f042189a1c
Author: Baineng Shou <[email protected]>

drm/prime: use dma_buf_fd_install() to preserve export tracing
    
This commit replaces fd_install() with dma_buf_fd_install() in
drm_gem_prime_handle_to_fd() to restore tracepoint coverage for DRM PRIME
exports. It asserts that the file descriptor lifecycle is already correct.

Link: https://lore.kernel.org/dri-devel/[email protected]/

> No functional change; the fd lifecycle (get_unused_fd_flags
> -> work -> install) is already correct.

[Severity: High]
This isn't a bug introduced by this patch, but is this claim about the fd
lifecycle being correct accurate?

The new dma_buf_fd_install() API requires that it must only be called after
all fallible work (such as copy_to_user) has succeeded.

> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 9b44c78cd77fc..fe3436d1235db 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
[ ... ]
> @@ -524,7 +524,7 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>  		return PTR_ERR(dmabuf);
>  	}
>  
> -	fd_install(fd, dmabuf->file);
> +	dma_buf_fd_install(dmabuf, fd);

[Severity: High]
This is a pre-existing issue, but by adopting the new dma_buf_fd_install()
API, doesn't this code path violate its documented contract?

Can this lead to a file descriptor leak if a userspace application calls the
DRM_IOCTL_PRIME_HANDLE_TO_FD ioctl with an invalid userspace pointer?

If drm_gem_prime_handle_to_fd() publishes the file descriptor to the
userspace fdtable here:

dma_buf_fd_install(dmabuf, fd);
*prime_fd = fd;
return 0;

Then the DRM core ioctl dispatcher attempts to copy the result to userspace
after the handler returns, which can fail:

drivers/gpu/drm/drm_ioctl.c:drm_ioctl() {
    ...
    retcode = func(dev, kdata, file_priv);
    ...
    if (copy_to_user((void __user *)arg, kdata, out_size) != 0)
        retcode = -EFAULT;
    ...
}

Wouldn't the failed copy_to_user() return -EFAULT and leave the already
installed fd orphaned in the process's fdtable, without the application
ever receiving the fd number to close it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3