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

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

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
>