Re: [PATCH] drm/amdgpu: Fix 'fw_name' buffer size to prevent truncations in amdgpu_mes_init_microcode

["Lazar, Lijo" <lijo.lazar@amd.com>]

On 3/21/2024 11:16 AM, Srinivasan Shanmugam wrote:
> The snprintf function is used to write a formatted string into fw_name.
> The format of the string is "amdgpu/%s_mes%s.bin", where %s is replaced
> by the string in ucode_prefix and the second %s is replaced by either
> "_2" or "1" depending on the condition pipe == AMDGPU_MES_SCHED_PIPE.
> 
> The length of the string "amdgpu/%s_mes%s.bin" is 16 characters plus the
> length of ucode_prefix and the length of the string "_2" or "1". The
> size of ucode_prefix is 30, so the maximum length of ucode_prefix is 29
> characters (since one character is needed for the null terminator).
> Therefore, the maximum possible length of the string written into
> fw_name is 16 + 29 + 2 = 47 characters.
> 
> The size of fw_name is 40, so if the length of the string written into
> fw_name is more than 39 characters (since one character is needed for
> the null terminator), it will be truncated by the snprintf function, and
> thus warnings will be seen.
> 
> By increasing the size of fw_name to 50, we ensure that fw_name is
> large enough to hold the maximum possible length of the string, so the
> snprintf function will not truncate the output.
> 
> Fixes the below with gcc W=1:
> drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c: In function ‘amdgpu_mes_init_microcode’:
> drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c:1482:66: warning: ‘%s’ directive output may be truncated writing up to 1 bytes into a region of size between 0 and 29 [-Wformat-truncation=]
>  1482 |                 snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_mes%s.bin",
>       |                                                                  ^~
> drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c:1482:17: note: ‘snprintf’ output between 16 and 46 bytes into a destination of size 40
>  1482 |                 snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_mes%s.bin",
>       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  1483 |                          ucode_prefix,
>       |                          ~~~~~~~~~~~~~
>  1484 |                          pipe == AMDGPU_MES_SCHED_PIPE ? "" : "1");
>       |                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c:1477:66: warning: ‘%s’ directive output may be truncated writing 1 byte into a region of size between 0 and 29 [-Wformat-truncation=]
>  1477 |                 snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_mes%s.bin",
>       |                                                                  ^~
>  1478 |                          ucode_prefix,
>  1479 |                          pipe == AMDGPU_MES_SCHED_PIPE ? "_2" : "1");
>       |                                                                 ~~~
> drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c:1477:17: note: ‘snprintf’ output between 17 and 46 bytes into a destination of size 40
>  1477 |                 snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_mes%s.bin",
>       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  1478 |                          ucode_prefix,
>       |                          ~~~~~~~~~~~~~
>  1479 |                          pipe == AMDGPU_MES_SCHED_PIPE ? "_2" : "1");
>       |                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c:1477:66: warning: ‘%s’ directive output may be truncated writing 2 bytes into a region of size between 0 and 29 [-Wformat-truncation=]
>  1477 |                 snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_mes%s.bin",
>       |                                                                  ^~
>  1478 |                          ucode_prefix,
>  1479 |                          pipe == AMDGPU_MES_SCHED_PIPE ? "_2" : "1");
>       |                                                          ~~~~
> drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c:1477:17: note: ‘snprintf’ output between 18 and 47 bytes into a destination of size 40
>  1477 |                 snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_mes%s.bin",
>       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  1478 |                          ucode_prefix,
>       |                          ~~~~~~~~~~~~~
>  1479 |                          pipe == AMDGPU_MES_SCHED_PIPE ? "_2" : "1");
>       |                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c:1489:62: warning: ‘_mes.bin’ directive output may be truncated writing 8 bytes into a region of size between 4 and 33 [-Wformat-truncation=]
>  1489 |                 snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_mes.bin",
>       |                                                              ^~~~~~~~
> drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c:1489:17: note: ‘snprintf’ output between 16 and 45 bytes into a destination of size 40
>  1489 |                 snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_mes.bin",
>       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  1490 |                          ucode_prefix);
>       |                          ~~~~~~~~~~~~~
> 
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Christian König <christian.koenig@amd.com>
> Suggested-by: Lijo Lazar <lijo.lazar@amd.com>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>

Reviewed-by: Lijo Lazar <lijo.lazar@amd.com>

Thanks,
Lijo
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c
> index bc8906403270..78dfd027dc99 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c
> @@ -1467,7 +1467,7 @@ int amdgpu_mes_init_microcode(struct amdgpu_device *adev, int pipe)
>  	const struct mes_firmware_header_v1_0 *mes_hdr;
>  	struct amdgpu_firmware_info *info;
>  	char ucode_prefix[30];
> -	char fw_name[40];
> +	char fw_name[50];
>  	bool need_retry = false;
>  	int r;
>