Am 11.04.2018 um 14:03 schrieb Tom St Denis:
>
>
> 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.
So pos=0 is one dw and pos=1 is another one? If that's the case then
that would be a bug since pos is by definition a byte offset.
I suggest to keep the limitation and instead fix how pos is interpreted
instead.
Regards,
Christian.
>
> 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
>>
