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
