Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [Critical] Unchecked dereference of `adev->ualink.info` in `amdgpu_gem_ualink_handle_ioctl` when UALink is unsupported. - [High] Lockless read and write of `robj->ualink_handle_lo` allows a TOCTOU race condition that permanently leaks BO references and XArray nodes. - [High] Missing `amdgpu_bo_unref()` on error path in `amdgpu_ualink_export_handle()` leads to GPU memory leak. -- commit ca49d768520fb1865985555b2647fee616d8460f Author: Karl Mehltretter <[email protected]> drm/amdgpu: Initialize UALink handle uniqueness state amdgpu_generate_ualink_handle() continues when handle_lo is zero. For accelerator ID 0, the random value can become zero after the low 10 bits are cleared. This path reaches while (!unique) without assigning unique. [ ... ] Initialize unique to false so the zero-handle path repeats the loop. Fixes: c67cb19811ec ("drm/amdgpu: Implement UALink handle export") Assisted-by: LLM Signed-off-by: Karl Mehltretter <[email protected]> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c > index ab564202f550a..c6b7122301f92 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c > @@ -1819,7 +1819,7 @@ static void amdgpu_ualink_npa_mm_fini(struct > amdgpu_device *adev) > static void amdgpu_generate_ualink_handle(struct amdgpu_device *adev, > struct amdgpu_ualink_handle *handle) > { > - bool unique; > + bool unique = false; > > do { > handle->handle_lo = get_random_u64(); While reviewing this patch, I noticed a few pre-existing issues in the same file: [Severity: Critical] This is a pre-existing issue, but does amdgpu_gem_ualink_handle_ioctl() panic on GPUs lacking UALink support? drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c:amdgpu_gem_ualink_handle_ioctl() { ... if (adev->ualink.info->accel_state != AMDGPU_UALINK_ACCEL_STATE_ACTIVE) { dev_err(adev->dev, "ualink device is not in active state in vpod\n"); return -EOPNOTSUPP; } ... } adev->ualink.info is only allocated if the GPU supports UALink. Since the DRM_IOCTL_AMDGPU_UALINK_HANDLE ioctl is registered unconditionally for the whole driver, can unprivileged userspace trigger an immediate kernel crash (NULL pointer dereference) by calling this ioctl on an unsupported GPU? [Severity: High] This isn't a bug introduced by this patch, but does amdgpu_ualink_export_handle() leak a BO and an exp_xa_node? drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c:amdgpu_ualink_export_handle() { ... if (!robj->ualink_handle_lo) { ... amdgpu_bo_ref(robj); ... xa_lock(&adev->ualink.exp_xa); amdgpu_generate_ualink_handle(adev, &handle); exp_xa_node->handle = handle; r = __xa_insert(&adev->ualink.exp_xa, handle.handle_lo, exp_xa_node, GFP_KERNEL); xa_unlock(&adev->ualink.exp_xa); ... robj->ualink_handle_lo = handle.handle_lo; ... } ... } Because robj->ualink_handle_lo is checked and overwritten locklessly (without holding a lock like the BO's reservation lock), could a TOCTOU race occur if two threads from the same process concurrently call the export ioctl for the same GEM handle? It seems both threads would observe robj->ualink_handle_lo == 0, allocate an exp_xa_node, call amdgpu_bo_ref(robj), insert their node into the exp_xa array, and locklessly overwrite robj->ualink_handle_lo. Since cleanup only removes the handle stored in ualink_handle_lo, one node remains permanently in the XArray and its BO reference is never released, leaking VRAM. [Severity: High] This is a pre-existing issue, but does amdgpu_ualink_export_handle() leak the BO reference on the DMABuf error path? drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c:amdgpu_ualink_export_handle() { ... amdgpu_bo_ref(robj); ... exp_xa_node->dmabuf = drm_gem_prime_handle_to_dmabuf(&adev->ddev, filp, gem_handle, DRM_CLOEXEC | DRM_RDWR); if (IS_ERR(exp_xa_node->dmabuf)) { r = PTR_ERR(exp_xa_node->dmabuf); dev_err(adev->dev, "Failed to generate DMABuf for the BO\n"); kfree(exp_xa_node); goto out; } ... out: drm_gem_object_put(gobj); return r; } The out label drops the GEM object reference via drm_gem_object_put(), but fails to balance the explicit amdgpu_bo_ref(robj) taken earlier. If drm_gem_prime_handle_to_dmabuf() fails (for example, due to file descriptor exhaustion resulting in -EMFILE), won't this permanently leak the BO reference? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
