[bug report] drm/amdkfd: Export DMABufs from KFD using GEM handles

[bug report] drm/amdkfd: Export DMABufs from KFD using GEM handles

[Dan Carpenter]

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.

   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

Re: [bug report] drm/amdkfd: Export DMABufs from KFD using GEM handles

[Felix Kuehling]

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