Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [Critical] The ioctl now fails with -EINVAL for callers requesting BO info without a metadata buffer, breaking backward compatibility. - [High] Missing dma_buf reference cleanup on the kzalloc failure path leads to a reference leak. Pre-existing issues: - [High] Lockless access to BO metadata may race with concurrent metadata updates, leading to a UAF and info leak. -- commit 98042267674366568e68066442c1f4f8b9968f33 Author: Xiaogang Chen <[email protected]> drm/amdkfd: Let driver decide buffer size at AMDKFD_IOC_GET_DMABUF_INFO ioctl This commit hardens the AMDKFD_IOC_GET_DMABUF_INFO ioctl by allocating the metadata buffer based on the driver-queried BO metadata size rather than a user-provided size. This prevents an issue where a malicious render-group user could provide an extremely large size to trigger a kernel OOM. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c [ ... ] > @@ -563,9 +563,24 @@ int amdgpu_amdkfd_get_dmabuf_info(struct amdgpu_device > *adev, int dma_buf_fd, > *dmabuf_adev = adev; > if (bo_size) > *bo_size = amdgpu_bo_size(bo); > - if (metadata_buffer) > - r = amdgpu_bo_get_metadata(bo, metadata_buffer, buffer_size, > - metadata_size, &metadata_flags); > + if (metadata_buffer) { > + /* first get metadata_size by buffer = NULL */ > + r = amdgpu_bo_get_metadata(bo, NULL, 0, > + metadata_size, NULL); [Severity: High] This is a pre-existing issue, but does lockless access to the BO metadata here race with concurrent metadata updates? Another thread could call amdgpu_gem_metadata_ioctl() which frees and reallocates ubo->metadata. If the free occurs while the inner memcpy() inside amdgpu_bo_get_metadata() is executing, it might read freed memory and copy it to userspace. Should the reservation lock (amdgpu_bo_reserve) be held before reading metadata here? > + > + /* user buf_size is bigger than bo metadata_size > + * allocate a buf at kernel space and copy */ > + if (*metadata_size <= buffer_size) { > + *metadata_buffer = kzalloc(*metadata_size, GFP_KERNEL); > + > + if (!*metadata_buffer) > + return -ENOMEM; [Severity: High] Does this direct return skip the out_put label and leak the dma_buf reference? If kzalloc() fails, returning -ENOMEM directly bypasses the dma_buf_put(dma_buf) call at the end of the function. > + > + r = amdgpu_bo_get_metadata(bo, *metadata_buffer, > *metadata_size, > + NULL, &metadata_flags); > + } else > + r = -EINVAL; > + } > if (flags) { > *flags = (bo->preferred_domains & AMDGPU_GEM_DOMAIN_VRAM) ? > KFD_IOC_ALLOC_MEM_FLAGS_VRAM > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c > b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c > --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c [ ... ] > @@ -1520,16 +1520,10 @@ static int kfd_ioctl_get_dmabuf_info(struct file > *filep, > if (!dev) > return -EINVAL; > > - if (args->metadata_ptr) { > - metadata_buffer = kzalloc(args->metadata_size, GFP_KERNEL); > - if (!metadata_buffer) > - return -ENOMEM; > - } > - > /* Get dmabuf info from KGD */ > r = amdgpu_amdkfd_get_dmabuf_info(dev->adev, args->dmabuf_fd, > &dmabuf_adev, &args->size, > - metadata_buffer, args->metadata_size, > + &metadata_buffer, args->metadata_size, [Severity: Critical] Will unconditionally passing this pointer break backwards compatibility for callers who explicitly opt out of fetching metadata? Previously, if args->metadata_ptr was 0, metadata_buffer was NULL, and amdgpu_amdkfd_get_dmabuf_info() would skip the metadata fetch entirely. Now, because &metadata_buffer is unconditionally passed, the pointer is never NULL inside amdgpu_amdkfd_get_dmabuf_info(). If userspace requests basic BO info without metadata by providing args->metadata_ptr = 0 and args->metadata_size = 0, but the BO actually contains metadata, the *metadata_size <= buffer_size condition in amdgpu_amdkfd_get_dmabuf_info() will fail. This causes the ioctl to unexpectedly return -EINVAL, even though the caller did not want the metadata in the first place. > &args->metadata_size, &flags, > &xcp_id); > if (r) > goto exit; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
