Re: [PATCH 6.1] drm/amdgpu: Use scnprintf() in amdgpu_mes_add_ring()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Double unlock of `adev->mes` mutex in error path.
- [High] Unchecked NULL fence pointers passed to `dma_fence_wait()`.
--

--- Patch [1]: [PATCH 6.1] drm/amdgpu: Use scnprintf() in amdgpu_mes_add_ring() ---
commit a3f6095725ba7df26135d6b5a8b649631f54d76e
Author: Evgenii Burenchev <evg28bur@yandex.ru>

drm/amdgpu: Use scnprintf() in amdgpu_mes_add_ring()

Replace sprintf() with scnprintf() to prevent a potential buffer overflow
when writing to ring->name.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c
> index 0c546245793b9..41e699f4b5d2e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c
> @@ -1053,13 +1053,14 @@ int amdgpu_mes_add_ring(struct amdgpu_device *adev, int gang_id,
>  	ring->doorbell_index = qprops.doorbell_off;
>  
>  	if (queue_type == AMDGPU_RING_TYPE_GFX)

[Severity: High]
This is a pre-existing issue, but I noticed a potential NULL pointer
dereference just before this code in amdgpu_mes_add_ring():

	dma_fence_wait(gang->process->vm->last_update, false);
	dma_fence_wait(ctx_data->meta_data_va->last_pt_update, false);

If the fences are unallocated, these pointers might be NULL. The
dma_fence_wait() function does not check for NULL inputs before dereferencing.
Could this result in a kernel oops during normal process/gang queue creation?

[Severity: High]
This is a pre-existing issue, but I also noticed a potential double unlock
of the adev->mes mutex. Before the new scnprintf() code, we unlock it here:

	amdgpu_mes_unlock(&adev->mes);

	r = amdgpu_mes_add_hw_queue(adev, gang_id, &qprops, &queue_id);
	if (r)
		goto clean_up_ring;

If amdgpu_mes_add_hw_queue() fails, it jumps to clean_up_ring, which falls
through to clean_up_memory:

clean_up_memory:
	kfree(ring);
	amdgpu_mes_unlock(&adev->mes);
	return r;

Will this second call to amdgpu_mes_unlock() cause a double unlock panic or
state corruption since the mutex was already dropped?

> -		sprintf(ring->name, "gfx_%d.%d.%d", pasid, gang_id, queue_id);
> +		scnprintf(ring->name, sizeof(ring->name), "gfx_%d.%d.%d",
> +			pasid, gang_id, queue_id);
>  	else if (queue_type == AMDGPU_RING_TYPE_COMPUTE)
> -		sprintf(ring->name, "compute_%d.%d.%d", pasid, gang_id,
> -			queue_id);
> +		scnprintf(ring->name, sizeof(ring->name), "compute_%d.%d.%d",
> +			pasid, gang_id, queue_id);
>  	else if (queue_type == AMDGPU_RING_TYPE_SDMA)
> -		sprintf(ring->name, "sdma_%d.%d.%d", pasid, gang_id,
> -			queue_id);
> +		scnprintf(ring->name, sizeof(ring->name), "sdma_%d.%d.%d",
> +			pasid, gang_id, queue_id);
>  	else
>  		BUG();
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709112825.40016-1-evg28bur@yandex.ru?part=1