Hello Alex Deucher,
The patch a2e73f56fa62: "drm/amdgpu: Add support for CIK parts" from Apr 20, 2015, leads to the following static checker warning:
drivers/gpu/drm/amd/amdgpu/ci_dpm.c:4509 ci_set_mc_special_registers() error: buffer overflow 'table->mc_reg_address' 16 <= 16
drivers/gpu/drm/amd/amdgpu/ci_dpm.c 4491 j++; 4492 if (j >= SMU7_DISCRETE_MC_REGISTER_ARRAY_SIZE) 4493 return -EINVAL; 4494 4495 temp_reg = RREG32(mmMC_PMG_CMD_MRS); 4496 table->mc_reg_address[j].s1 = mmMC_PMG_CMD_MRS; 4497 table->mc_reg_address[j].s0 = mmMC_SEQ_PMG_CMD_MRS_LP; 4498 for (k = 0; k < table->num_entries; k++) { 4499 table->mc_reg_table_entry[k].mc_data[j] = 4500 (temp_reg & 0xffff0000) | (table->mc_reg_table_entry[k].mc_data[i] & 0x0000ffff); 4501 if (adev->mc.vram_type != AMDGPU_VRAM_TYPE_GDDR5) 4502 table->mc_reg_table_entry[k].mc_data[j] |= 0x100; 4503 } 4504 j++; 4505 if (j > SMU7_DISCRETE_MC_REGISTER_ARRAY_SIZE)
This should >= instead of >.
4506 return -EINVAL; 4507 4508 if (adev->mc.vram_type != AMDGPU_VRAM_TYPE_GDDR5) { 4509 table->mc_reg_address[j].s1 = mmMC_PMG_AUTO_CMD; 4510 table->mc_reg_address[j].s0 = mmMC_PMG_AUTO_CMD; 4511 for (k = 0; k < table->num_entries; k++) { 4512 table->mc_reg_table_entry[k].mc_data[j] = 4513 (table->mc_reg_table_entry[k].mc_data[i] & 0xffff0000) >> 16; 4514 } 4515 j++; 4516 if (j > SMU7_DISCRETE_MC_REGISTER_ARRAY_SIZE) 4517 return -EINVAL;
But my question was about the other checks, should they be changed as well?
4518 } 4519 break; 4520 case mmMC_SEQ_RESERVE_M: 4521 temp_reg = RREG32(mmMC_PMG_CMD_MRS1); 4522 table->mc_reg_address[j].s1 = mmMC_PMG_CMD_MRS1; 4523 table->mc_reg_address[j].s0 = mmMC_SEQ_PMG_CMD_MRS1_LP; 4524 for (k = 0; k < table->num_entries; k++) { 4525 table->mc_reg_table_entry[k].mc_data[j] = 4526 (temp_reg & 0xffff0000) | (table->mc_reg_table_entry[k].mc_data[i] & 0x0000ffff); 4527 } 4528 j++; 4529 if (j > SMU7_DISCRETE_MC_REGISTER_ARRAY_SIZE) 4530 return -EINVAL; 4531 break; 4532 default: 4533 break; 4534 } 4535 4536 } 4537 4538 table->last = j;
We use the last j here. It's not clear to me if ->last has to be within the array. I would have generally assumed it would be except for the check below.
4539 4540 return 0; 4541 } 4542
[ snip ]
4670 static int ci_register_patching_mc_seq(struct amdgpu_device *adev, 4671 struct ci_mc_reg_table *table) 4672 { 4673 u8 i, k; 4674 u32 tmp; 4675 bool patch; 4676 4677 tmp = RREG32(mmMC_SEQ_MISC0); 4678 patch = ((tmp & 0x0000f00) == 0x300) ? true : false; 4679 4680 if (patch && 4681 ((adev->pdev->device == 0x67B0) || 4682 (adev->pdev->device == 0x67B1))) { 4683 for (i = 0; i < table->last; i++) { 4684 if (table->last >= SMU7_DISCRETE_MC_REGISTER_ARRAY_SIZE)
Here, we assume ->last has to be inside the array.
4685 return -EINVAL;
regards, dan carpenter