[PATCH] drm/amd/amdgpu: Fix amdgpu_debugfs_gpr_read debugfs entry

[Christian König]

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
>>