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@xxxxxxx>
Cc: Christian König <christian.koenig@xxxxxxx>
Suggested-by: Lijo Lazar <lijo.lazar@xxxxxxx>
Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@xxxxxxx>
---
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++) {
