re: drm/amdgpu: Add support for CIK parts

re: drm/amdgpu: Add support for CIK parts

[Dan Carpenter]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel