On 5/21/2024 1:07 PM, 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 '%hhu' format
> specifier for ring->me, ring->pipe, and ring->queue. The '%hhu'
> specifier is used for unsigned char variables and ensures that these
> values are printed as unsigned decimal integers.
>
> 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@xxxxxxx>
> Cc: Christian König <christian.koenig@xxxxxxx>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@xxxxxxx>
> ---
> v2:
> - Removed width specifiers %3, %1, typecasting of unsigned char,
> s/hhd/hhu (Lijo)
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> index 9b7dc61c331d..0f14d4a11441 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> @@ -329,7 +329,7 @@ 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",
> + snprintf(ring->name, sizeof(ring->name), "kiq_%d.%hhu.%hhu.%hhu",
> xcc_id, ring->me, ring->pipe, ring->queue);
Even for xcc_id, the value range expected is < 255. Anyway,
Reviewed-by: Lijo Lazar <lijo.lazar@xxxxxxx>
Thanks,
Lijo
> r = amdgpu_ring_init(adev, ring, 1024, irq, AMDGPU_CP_KIQ_IRQ_DRIVER0,
> AMDGPU_RING_PRIO_DEFAULT, NULL);
