Re: [PATCH] drm/amdgpu: Fix format character cut-off issues in amdgpu_vcn_early_init()

[SRINIVASAN SHANMUGAM <srinivasan.shanmugam@amd.com>]

On 3/20/2024 3:12 PM, Lazar, Lijo wrote:
>
> On 3/20/2024 2:15 PM, Srinivasan Shanmugam wrote:
>> The issue was present in the lines where 'fw_name' was being formatted.
>> This fix ensures that the output is not truncated
>>
>> Fixes the below with gcc W=1:
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c: In function ‘amdgpu_vcn_early_init’:
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c:102:66: warning: ‘snprintf’ output may be truncated before the last format character [-Wformat-truncation=]
>>    102 |                 snprintf(fw_name, sizeof(fw_name), "amdgpu/%s.bin", ucode_prefix);
>>        |                                                                  ^
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c:102:17: note: ‘snprintf’ output between 12 and 41 bytes into a destination of size 40
>>    102 |                 snprintf(fw_name, sizeof(fw_name), "amdgpu/%s.bin", ucode_prefix);
>>        |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c:102:66: warning: ‘snprintf’ output may be truncated before the last format character [-Wformat-truncation=]
>>    102 |                 snprintf(fw_name, sizeof(fw_name), "amdgpu/%s.bin", ucode_prefix);
>>        |                                                                  ^
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c:102:17: note: ‘snprintf’ output between 12 and 41 bytes into a destination of size 40
>>    102 |                 snprintf(fw_name, sizeof(fw_name), "amdgpu/%s.bin", ucode_prefix);
>>        |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c:105:73: warning: ‘.bin’ directive output may be truncated writing 4 bytes into a region of size between 2 and 31 [-Wformat-truncation=]
>>    105 |                         snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_%d.bin", ucode_prefix, i);
>>        |                                                                         ^~~~
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c:105:25: note: ‘snprintf’ output between 14 and 43 bytes into a destination of size 40
>>    105 |                         snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_%d.bin", ucode_prefix, i);
>>        |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> 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>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>> index 9c514a606a2f..f994ee6c548d 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>> @@ -94,7 +94,7 @@ static void amdgpu_vcn_idle_work_handler(struct work_struct *work);
>>   int amdgpu_vcn_early_init(struct amdgpu_device *adev)
>>   {
>>   	char ucode_prefix[30];
> Hi Srini,
>
> Sorry, if I miscommunicated. Suggestion was to reduce prefix size to 25
> as the max prefix length is possibly length of dimgrey_cavefish_vcn.
Hi Lijo,

my mistake, the fw_name size must have been 53.

How 53? -> 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). The maximum number of digits in an integer is 10. 
Therefore, the maximum possible length of the string written into 
fw_name is 14 + 29 + 10 = 53 characters.

On the other hand reducing ucode_prefix to 25 from 30:

1. The length of the string "amdgpu/%s.bin" is 12 characters plus the 
length of ucode_prefix. The length of the string "amdgpu/%s_%d.bin" is 
14 characters plus the length of ucode_prefix and the number of digits in i.

If ucode_prefix is 25 characters long, the maximum length of the string 
written into fw_name is 14 + 25 + 1 (for a single digit i) = 40 
characters, which is exactly the size of fw_name.

Is that this solution assumes that i will not be more than 9 (a single 
digit)?. If i can be a number with more than one digit, should we need 
to increase the size of fw_name accordingly.?

2. If you reduce the size of ucode_prefix to 25, it means that it can 
store a version string of up to 24 characters (since one character is 
needed for the null terminator).

For example: if tomorrow, if we get something like 
dimgrey_cavefish_smc_xxxyyyzz - then in this case yyyzz would be lost? 
is that in "amdgpu_ucode_ip_version_decode " & 
"amdgpu_ucode_legacy_naming" , is that are we always ensuring that it 
will never be longer than 24 characters?

Thanks,
Srini
>
> With fw_name[42] also, you may run into 43 bytes (30 prefix + 13 for
> others) warning.
>
> Thanks,
> Lijo
>
>> -	char fw_name[40];
>> +	char fw_name[42];
>>   	int r, i;
>>   
>>   	for (i = 0; i < adev->vcn.num_vcn_inst; i++) {