Re: [bug report] drm/amd/amdgpu: Add debugfs support for reading GPRs (v2)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Sorry about missing that.  A fix was sent to the list a few mins ago.  It also highlighted a bug in umr's reading of trap registers.  It's a genuine two-fer!

Tom


On 2020-03-10 8:23 a.m., Dan Carpenter wrote:
On Tue, Nov 28, 2017 at 09:37:44AM -0500, Tom St Denis wrote:
On 28/11/17 09:29 AM, Dan Carpenter wrote:
Hello Tom St Denis,

The patch c5a60ce81b49: "drm/amd/amdgpu: Add debugfs support for
reading GPRs (v2)" from Dec 5, 2016, leads to the following static
checker warning:

	drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:3774 amdgpu_debugfs_gpr_read()
	error: buffer overflow 'data' 1024 <= 4095

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
    3731  static ssize_t amdgpu_debugfs_gpr_read(struct file *f, char __user *buf,
    3732                                          size_t size, loff_t *pos)
    3733  {
    3734          struct amdgpu_device *adev = f->f_inode->i_private;
    3735          int r;
    3736          ssize_t result = 0;
    3737          uint32_t offset, se, sh, cu, wave, simd, thread, bank, *data;
    3738
    3739          if (size & 3 || *pos & 3)
    3740                  return -EINVAL;
    3741
    3742          /* decode offset */
    3743          offset = *pos & GENMASK_ULL(11, 0);
                  ^^^^^^
offset is set to 0-4095.

    3744          se = (*pos & GENMASK_ULL(19, 12)) >> 12;
    3745          sh = (*pos & GENMASK_ULL(27, 20)) >> 20;
    3746          cu = (*pos & GENMASK_ULL(35, 28)) >> 28;
    3747          wave = (*pos & GENMASK_ULL(43, 36)) >> 36;
    3748          simd = (*pos & GENMASK_ULL(51, 44)) >> 44;
    3749          thread = (*pos & GENMASK_ULL(59, 52)) >> 52;
    3750          bank = (*pos & GENMASK_ULL(61, 60)) >> 60;
    3751
    3752          data = kmalloc_array(1024, sizeof(*data), GFP_KERNEL);
                                       ^^^^
data is a 1024 element array

    3753          if (!data)
    3754                  return -ENOMEM;
    3755
    3756          /* switch to the specific se/sh/cu */
    3757          mutex_lock(&adev->grbm_idx_mutex);
    3758          amdgpu_gfx_select_se_sh(adev, se, sh, cu);
    3759
    3760          if (bank == 0) {
    3761                  if (adev->gfx.funcs->read_wave_vgprs)
    3762                          adev->gfx.funcs->read_wave_vgprs(adev, simd, wave, thread, offset, size>>2, data);
    3763          } else {
    3764                  if (adev->gfx.funcs->read_wave_sgprs)
    3765                          adev->gfx.funcs->read_wave_sgprs(adev, simd, wave, offset, size>>2, data);
    3766          }
    3767
    3768          amdgpu_gfx_select_se_sh(adev, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF);
    3769          mutex_unlock(&adev->grbm_idx_mutex);
    3770
    3771          while (size) {
    3772                  uint32_t value;
    3773
    3774                  value = data[offset++];
                                  ^^^^^^^^^^^^^^
We're possibly reading beyond the end of the array.  Maybe we are
supposed to divide the offset /= sizeof(*data)?
Hi Dan,


umr only reads from offset zero but to be consistent I think yes the offset
should be /= 4 first since we ensure it's 4 byte aligned it's clearly a 4
byte offset.

Thanks for finding this, I'll craft up a patch shortly.

What ever happened with this?

regards,
dan carpenter

_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux