Re: [PATCH next] drm/amdgpu: Initialize UALink handle uniqueness state

[sashiko-bot@kernel.org]

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 <kmehltretter@gmail.com>

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 <kmehltretter@gmail.com>

> 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/20260914054604.12850-1-kmehltretter@gmail.com?part=1