On 2024-01-23 5:21, Dan Carpenter wrote:

Hello Felix Kuehling,

The patch 1819200166ce: "drm/amdkfd: Export DMABufs from KFD using GEM handles" from Aug 24, 2023 (linux-next), leads to the following Smatch static checker warning:

drivers/dma-buf/dma-buf.c:729 dma_buf_get() warn: fd used after fd_install() 'fd'

drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 809 static int kfd_mem_export_dmabuf(struct kgd_mem *mem) 810 { 811 if (!mem->dmabuf) { 812 struct amdgpu_device *bo_adev; 813 struct dma_buf *dmabuf; 814 int r, fd; 815 816 bo_adev = amdgpu_ttm_adev(mem->bo->tbo.bdev); 817 r = drm_gem_prime_handle_to_fd(&bo_adev->ddev, bo_adev->kfd.client.file, 818 mem->gem_handle, 819 mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE ? 820 DRM_RDWR : 0, &fd); ^^^ The drm_gem_prime_handle_to_fd() function does an fd_install() and returns the result as "fd".

821                  if (r)
822                          return r;
823                  dmabuf = dma_buf_get(fd);
                                          ^^

Then we do another fget() inside dma_buf_get(). I'm not an expert, but this looks wrong. We can't assume that the dmabuf here is the same one from drm_gem_prime_handle_to_fd() because the user could change it after the fd_install(). I suspect drm_gem_prime_handle_to_fd() should pass the dmabuf back instead.

We had several CVEs similar to this such as CVE-2022-1998.

That CVE is a system crash. I don't think that can happen here. It's true that user mode can close the fd. But then dma_buf_get would return an error that we'd catch with "WARN_ON_ONCE(IS_ERR(dmabuf))" below.

It's possible that a different DMABuf gets bound to the fd by a malicious user mode. That said, we're treating this fd as if it had come from user mode. dma_buf_get and the subsequent check on the dmabuf should be robust against any user mode messing with the file descriptor in the meantime.

Regards,   Felix

824                  close_fd(fd);
825                  if (WARN_ON_ONCE(IS_ERR(dmabuf)))
826                          return PTR_ERR(dmabuf);
827                  mem->dmabuf = dmabuf;
828          }
829
830          return 0;
831  }

regards, dan carpenter