On 04/11/2018 07:58 AM, Christian König wrote:
>>
>> Â -Â Â Â if (size & 3 || *pos & 3)
>> +Â Â Â if (size & 3 || size > (4 * AMDGPU_DEBUGFS_MAX_SGPR_READ))
>
> I think checking the position alignment here is still necessary, cause
> we can't read from not dw boundaries don't we?
The index is a dword index as fed into SQ_IND_INDEX (offset => start =>
regno as you trace from the debugfs entry to gfx_v8_0_read_wave_sgprs to
wave_read_regs (or analogues...)).
SQ_IND_INDEX doesn't take a byte offset but a dword offset, for instance:
include/asic_reg/gc/gc_9_0_offset.h:#define ixSQ_WAVE_TTMP0
0x026c
include/asic_reg/gc/gc_9_0_offset.h:#define ixSQ_WAVE_TTMP1
0x026d
include/asic_reg/gc/gc_9_0_offset.h:#define ixSQ_WAVE_TTMP2
0x026e
The current way for instance would prohibit reading (directly)
SQ_WAVE_TTMP1.
I agree it's not really how a typical file device works but it's not a
typical file device :-). It's assumed every read would be preceded by a
seek to set the higher order bits anyways.
Cheers,
Tom
>
> Christian.
>
> Am 11.04.2018 um 13:55 schrieb Tom St Denis:
>> Ping?
>>
>> On 04/09/2018 08:16 AM, Tom St Denis wrote:
>>> We don't need to check the alignment of the offset and there was
>>> potential a buffer overflow as well.
>>>
>>> Signed-off-by: Tom St Denis <tom.stdenis at amd.com>
>>> ---
>>> Â drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 8 ++++++--
>>> Â 1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> index c98e59721444..b1ea300008e5 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> @@ -507,6 +507,9 @@ static ssize_t amdgpu_debugfs_wave_read(struct
>>> file *f, char __user *buf,
>>> Â Â Â Â Â return result;
>>> Â }
>>> Â +// read at most 1024 words
>>> +#define AMDGPU_DEBUGFS_MAX_SGPR_READ 1024
>>> +
>>> Â static ssize_t amdgpu_debugfs_gpr_read(struct file *f, char __user
>>> *buf,
>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â size_t size, loff_t *pos)
>>> Â {
>>> @@ -515,7 +518,7 @@ static ssize_t amdgpu_debugfs_gpr_read(struct
>>> file *f, char __user *buf,
>>> Â Â Â Â Â ssize_t result = 0;
>>> Â Â Â Â Â uint32_t offset, se, sh, cu, wave, simd, thread, bank, *data;
>>> Â -Â Â Â if (size & 3 || *pos & 3)
>>> +Â Â Â if (size & 3 || size > (4 * AMDGPU_DEBUGFS_MAX_SGPR_READ))
>>> Â Â Â Â Â Â Â Â Â return -EINVAL;
>>> Â Â Â Â Â Â /* decode offset */
>>> @@ -528,7 +531,8 @@ static ssize_t amdgpu_debugfs_gpr_read(struct
>>> file *f, char __user *buf,
>>> Â Â Â Â Â thread = (*pos & GENMASK_ULL(59, 52)) >> 52;
>>> Â Â Â Â Â bank = (*pos & GENMASK_ULL(61, 60)) >> 60;
>>> Â -Â Â Â data = kmalloc_array(1024, sizeof(*data), GFP_KERNEL);
>>> +Â Â Â data = kmalloc_array(AMDGPU_DEBUGFS_MAX_SGPR_READ, sizeof(*data),
>>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â GFP_KERNEL);
>>> Â Â Â Â Â if (!data)
>>> Â Â Â Â Â Â Â Â Â return -ENOMEM;
>>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
