All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dan Carpenter <dan.carpenter@oracle.com>
To: alexander.deucher@amd.com
Cc: dri-devel@lists.freedesktop.org
Subject: re: drm/amdgpu: Add support for CIK parts
Date: Thu, 11 Jun 2015 18:04:25 +0300	[thread overview]
Message-ID: <20150611150425.GA12192@mwanda> (raw)

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

                 reply	other threads:[~2015-06-11 15:04 UTC|newest]

Thread overview: [no followups] expand[flat|nested]  mbox.gz  Atom feed

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150611150425.GA12192@mwanda \
    --to=dan.carpenter@oracle.com \
    --cc=alexander.deucher@amd.com \
    --cc=dri-devel@lists.freedesktop.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.