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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
