Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Missing buffer object reservation lock in `amdgpu_amdkfd_get_dmabuf_info()` leads to a Use-After-Free or Data Race when accessing BO metadata. - [High] Dereferencing dmabuf_adev->kfd.dev can cause a kernel panic if the imported DMA buffer was exported from an AMDGPU device without KFD initialized. -- commit b918bf448d8c4caa5149dad7e3a094a3b15f65eb Author: Christophe JAILLET <[email protected]> drm/amdkfd: Fix an error handling path in amdgpu_amdkfd_get_dmabuf_info() This commit fixes an error handling path in amdgpu_amdkfd_get_dmabuf_info() by properly releasing the reference on 'dma_buf' when memory allocation fails. This aligns with the error handling in other paths of the function. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c > index d516d504ae4c8..8e7212a03475c 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c > @@ -603,8 +603,10 @@ int amdgpu_amdkfd_get_dmabuf_info(struct amdgpu_device > *adev, int dma_buf_fd, > if (*metadata_size <= buffer_size) { > *metadata_buffer = kzalloc(*metadata_size, GFP_KERNEL); > > - if (!*metadata_buffer) > - return -ENOMEM; > + if (!*metadata_buffer) { > + r = -ENOMEM; > + goto out_put; > + } > > r = amdgpu_bo_get_metadata(bo, *metadata_buffer, > *metadata_size, > NULL, &metadata_flags); [Severity: High] This isn't a bug introduced by this patch, but does amdgpu_amdkfd_get_dmabuf_info() need to acquire the buffer object's reservation lock before calling amdgpu_bo_get_metadata()? Concurrently, a userspace process can trigger the AMDGPU_GEM_METADATA_OP_SET_METADATA ioctl, which acquires the lock and calls amdgpu_bo_set_metadata(). This setter unconditionally calls kfree(ubo->metadata) and reallocates it. The lockless reader path in amdgpu_bo_get_metadata() will then execute a memcpy using the freed pointer: drivers/gpu/drm/amd/amdgpu/amdgpu_object.c:amdgpu_bo_get_metadata() { ... if (ubo->metadata_size) memcpy(buffer, ubo->metadata, ubo->metadata_size); ... } Could this lead to a use-after-free or out-of-bounds read if ubo->metadata is freed concurrently? [Severity: High] This is a pre-existing issue, but could the dmabuf_adev resolved in amdgpu_amdkfd_get_dmabuf_info() cause a NULL pointer dereference in its caller? In kfd_ioctl_get_dmabuf_info() located in drivers/gpu/drm/amd/amdkfd/kfd_chardev.c, amdgpu_amdkfd_get_dmabuf_info() successfully resolves dmabuf_adev to the exporting AMDGPU device. However, if KFD is not initialized on the exporting device, dmabuf_adev->kfd.dev will be NULL. Since xcp_id is typically initialized to 0, the check passes and leads to a dereference: drivers/gpu/drm/amd/amdkfd/kfd_chardev.c:kfd_ioctl_get_dmabuf_info() { ... if (xcp_id >= 0) args->gpu_id = dmabuf_adev->kfd.dev->nodes[xcp_id]->id; ... } Would it be possible for a local user to trigger this by passing a valid DMA buffer FD from a non-compute AMDGPU device to the KFD ioctl? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/44cbc1d12fc00121d40ce793074bc5e1d497c932.1781982199.git.christophe.jail...@wanadoo.fr?part=1
