[PATCH] drm/amdgpu: Fix snprintf usage in amdgpu_gfx_kiq_init_ring

[Lazar, Lijo]

On 5/21/2024 10:13 AM, Srinivasan Shanmugam wrote:
> This commit fixes a format truncation issue arosed by the snprintf
> function potentially writing more characters into the ring->name buffer
> than it can hold, in the amdgpu_gfx_kiq_init_ring function
> 
> The issue occurred because the '%d' format specifier could write between
> 1 and 10 bytes into a region of size between 0 and 8, depending on the
>   values of xcc_id, ring->me, ring->pipe, and ring->queue. The snprintf
> function could output between 12 and 41 bytes into a destination of size
> 16, leading to potential truncation.
> 
> To resolve this, the snprintf line was modified to use the '%3d' and
> '%1hhd' format specifiers. The '%3d' specifier is used for xcc_id and
> ensures that it is always printed with a width of 3 characters. The> '%1hhd' specifier is used for ring->me, ring->pipe, and ring->queue, and


Width specifier only guarantees minimum width. It doesn't offer any
truncation. %1 also doesn't matter as that is the default minimum. What
about just using %hhu?

Thanks,
Lijo


> ensures that these values are printed as single digit numbers. This is
> achieved by casting these values to unsigned char before passing them to
> snprintf, which ensures that these values will always be in the range of
> 0 to 9.
> 
> Fixes the below with gcc W=1:
> drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c: In function ‘amdgpu_gfx_kiq_init_ring’:
> drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c:332:61: warning: ‘%d’ directive output may be truncated writing between 1 and 10 bytes into a region of size between 0 and 8 [-Wformat-truncation=]
>   332 |         snprintf(ring->name, sizeof(ring->name), "kiq_%d.%d.%d.%d",
>       |                                                             ^~
> drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c:332:50: note: directive argument in the range [0, 2147483647]
>   332 |         snprintf(ring->name, sizeof(ring->name), "kiq_%d.%d.%d.%d",
>       |                                                  ^~~~~~~~~~~~~~~~~
> drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c:332:9: note: ‘snprintf’ output between 12 and 41 bytes into a destination of size 16
>   332 |         snprintf(ring->name, sizeof(ring->name), "kiq_%d.%d.%d.%d",
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   333 |                  xcc_id, ring->me, ring->pipe, ring->queue);
>       |                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Fixes: 345a36c4f1ba ("drm/amdgpu: prefer snprintf over sprintf")
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Cc: Christian König <christian.koenig at amd.com>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam at amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> index 9b7dc61c331d..88da17c0340b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> @@ -329,8 +329,9 @@ int amdgpu_gfx_kiq_init_ring(struct amdgpu_device *adev, int xcc_id)
>  
>  	ring->eop_gpu_addr = kiq->eop_gpu_addr;
>  	ring->no_scheduler = true;
> -	snprintf(ring->name, sizeof(ring->name), "kiq_%d.%d.%d.%d",
> -		 xcc_id, ring->me, ring->pipe, ring->queue);
> +	snprintf(ring->name, sizeof(ring->name), "kiq_%3d.%1hhd.%1hhd.%1hhd",
> +		 xcc_id, (unsigned char)ring->me, (unsigned char)ring->pipe,
> +		 (unsigned char)ring->queue);
>  	r = amdgpu_ring_init(adev, ring, 1024, irq, AMDGPU_CP_KIQ_IRQ_DRIVER0,
>  			     AMDGPU_RING_PRIO_DEFAULT, NULL);
>  	if (r)