* [bug report] drm/amd/amdgpu: Add debugfs support for reading GPRs (v2)
@ 2017-11-28 14:29 Dan Carpenter
2017-11-28 14:37 ` Tom St Denis
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2017-11-28 14:29 UTC (permalink / raw)
To: tom.stdenis-5C7GfCeVMHo; +Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
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)?
3775 r = put_user(value, (uint32_t *)buf);
regards,
dan carpenter
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] drm/amd/amdgpu: Add debugfs support for reading GPRs (v2)
2017-11-28 14:29 [bug report] drm/amd/amdgpu: Add debugfs support for reading GPRs (v2) Dan Carpenter
@ 2017-11-28 14:37 ` Tom St Denis
2020-03-10 12:23 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Tom St Denis @ 2017-11-28 14:37 UTC (permalink / raw)
To: Dan Carpenter; +Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] drm/amd/amdgpu: Add debugfs support for reading GPRs (v2)
2017-11-28 14:37 ` Tom St Denis
@ 2020-03-10 12:23 ` Dan Carpenter
2020-03-10 12:58 ` Tom St Denis
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2020-03-10 12:23 UTC (permalink / raw)
To: Tom St Denis; +Cc: amd-gfx
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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] drm/amd/amdgpu: Add debugfs support for reading GPRs (v2)
2020-03-10 12:23 ` Dan Carpenter
@ 2020-03-10 12:58 ` Tom St Denis
0 siblings, 0 replies; 4+ messages in thread
From: Tom St Denis @ 2020-03-10 12:58 UTC (permalink / raw)
To: Dan Carpenter; +Cc: amd-gfx
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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-03-10 12:58 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-28 14:29 [bug report] drm/amd/amdgpu: Add debugfs support for reading GPRs (v2) Dan Carpenter
2017-11-28 14:37 ` Tom St Denis
2020-03-10 12:23 ` Dan Carpenter
2020-03-10 12:58 ` Tom St Denis
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.