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

[Tom St Denis]

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.

Cheers,
Tom